-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
aggregators: refactor, introduce Writer #36
Conversation
Introduce a Writer type, which encapsulates a pebble.Batch and has methods for writing metrics to the batch. Splitting this out of the Aggregator type enables clients to create independent Writers which are bound to an execution context, to naturally avoid lock contention. Replace Aggregator.Stop with Aggregator.Close, which should always be called, even if periodic harvesting is never started. Replace Aggregator.Run with Aggregator.StartHarvesting, which starts the harvest loop in the background with no context; it will be stopped by Aggregator.Close. Move cachedEventsMap to its own file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave a brief look over the code. Do you think it would be a good idea to move the writers
to an internal package? It would make the interaction between aggregator and writer a bit more cleaner I think.
} | ||
batch := w.batch | ||
w.batch = nil | ||
w.aggregator.writers.Delete(w) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move managing the writers
to the aggregator
code? Also, maybe it would be beneficial to move the writer to internal
package to make the interface (public and private methods) a bit more cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move managing the writers to the aggregator code?
Do you mean move move the Writer.Close
method into aggregator?
Also, maybe it would be beneficial to move the writer to internal package to make the interface (public and private methods) a bit more cleaner?
There's not a lot of code in writer.go
, so I don't personally see value in making an internal package. I'm not sure I follow though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not a lot of code in writer.go, so I don't personally see value in making an internal package. I'm not sure I follow though.
Sorry, should have explained in more detail. The interaction between writer and aggregator seems a bit cluttered since they use the private and the public methods as required. I was suggesting moving the writer
to internal
in its separate package so that the interaction is cleaner (the exposed public methods are called between writer and aggregator).
Do you mean move move the Writer.Close method into aggregator?
I was referring to removing the access of writers
map from writer.go. Maybe we can return the batch from Close
and delete if from map in the aggregator
code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, should have explained in more detail. The interaction between writer and aggregator seems a bit cluttered since they use the private and the public methods as required. I was suggesting moving the writer to internal in its separate package so that the interaction is cleaner (the exposed public methods are called between writer and aggregator).
I had a look, but it's not straightforward since the writer needs to refer to the CombinedMetricsKey type.
I was referring to removing the access of writers map from writer.go. Maybe we can return the batch from Close and delete if from map in the aggregator code.
That won't allow a direct call to Writer.Close
to remove the writer from the Aggregator. A couple of options:
- We can not remove from the map eagerly, but defer that to harvest or Aggregator.Close. I don't love this since it means resources hang around unnecessarily.
- We could inject a callback into Writer, to be called on Close
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could inject a callback into Writer, to be called on Close
This sounds good but it might be problematic to pass callback in case we want to close the writer independently of the aggregator.
|
||
mu sync.Mutex | ||
mu sync.RWMutex | ||
writers sync.Map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that writers
is always accessed within Aggregator#mu#Lock
. Maybe we don't need the sync.Map
? I think we should also be good with a slice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That mutex isn't necessarily held when closing a writer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a slice requires O(N) writer removal though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just a map[*Writer]struct{}
, given it is protected by the aggregator mutex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(oh I'm late to the party, ignore my comments. Thanks for @axw 's explanation. I see that Writer.Close only holds w.mu.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that Writer.Close only holds w.mu.)
Yes but the caller to Writer.Close
i.e. Aggregator.Close
gets a lock. I was suggesting to keep the access of writers
to only the aggregator code and remove the requirement of sync based structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We remove all at one time, we can just iterate over the slice from the end and mark each of them nil. No need for O(N) I think.
I meant in the case of one writer closing itself, not aggregator.Close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but the caller to Writer.Close i.e. Aggregator.Close gets a lock.
I believe user code will call Writer.Close
as well, not only Aggregator.Close
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe user code will call Writer.Close as well, not only Aggregator.Close
Right. At least in theory, you could have multiple independent writers and they can each independently close their writers. For example, see BenchmarkAggregateBatch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe user code will call Writer.Close as well, not only Aggregator.Close.
Ah, good point, didn't realize this.
@lahsivjar could you please elaborate on that? I'm not picturing how that would look. |
// is closed. | ||
// | ||
// NewWriter will return an error if the aggregator has already been stopped. | ||
func (a *Aggregator) NewWriter() (*Writer, error) { | ||
a.mu.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RLock
should be sufficient but it leads to worse lock contention and worse performance in other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's very surprising, given that NewWriter should not be in any hot code paths...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
under BenchmarkAggregateBatchParallel with -cpu=512 it is. But I guess that's too extreme having many new writers in a second.
Introduce a Writer type, which encapsulates a pebble.Batch and has methods for writing metrics to the batch. Splitting this out of the Aggregator type enables clients to create independent Writers which are bound to an execution context, to naturally avoid lock contention.
AggregateBatch is now known as WriteEventMetrics, and AggregateCombinedMetrics is now known as WriteCombinedMetrics. We remove tracing from WriteCombinedMetrics, to bring it closer in line with WriteEventMetrics; the cost of tracing at this level is too great.
Replace Aggregator.Stop with Aggregator.Close, which should always be called, even if periodic harvesting is never started.
Replace Aggregator.Run with Aggregator.StartHarvesting, which starts the harvest loop in the background with no context; it will be stopped by Aggregator.Close.
Move cachedEventsMap to its own file.