Reduce method invocation of reservoir sampling#11257
Conversation
clintropolis
left a comment
clintropolis
left a comment
There was a problem hiding this comment.
It would be useful to add a benchmark for this code if possible to show the difference of the new implementation, maybe here: https://github.com/apache/druid/tree/master/benchmarks/src/test/java/org/apache/druid/server/coordinator
There was a problem hiding this comment.
nit: typo segmetnsToMove -> segmentsToMove
There was a problem hiding this comment.
does it make sense to retain the percentOfSegmentsToConsider functionality to allow short-circuiting iterateAllSegments across all servers? We update the docs https://github.com/apache/druid/blob/master/docs/configuration/index.md#dynamic-configuration accordingly either way
There was a problem hiding this comment.
It makes sense to me to retain the percentOfSegmentsToConsider and add an another dynamic parameter useBatchedSegmentSampler to enable the new improvement. In this way, the changing will be less aggressive.
There was a problem hiding this comment.
i think it would be worth adding some code comments here to describe that this algorithm has 2 phases, where it picks the first k segments from the servers, in order, then iterates through the server list randomly replacing these picked segments with decreasing frequency the more segments have been iterated.
Im also somewhat curious/nervous about random this pick method is compared to the previous one, though I'm not entirely sure how it would be measured.
I think we should add a new coordinator dynamic config property, https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java, to allow falling back to the old algorithm in case there are any unpredictable strange behavior caused by this new algorithm, maybe useBatchedSegmentSampler or .. i'm not sure, naming is hard.
There was a problem hiding this comment.
Random picking here is a standard implementation of a reservoir sampling algorithm. It makes all segments have the same probability to be selected. The original code also uses this trick, so no need to worry :-)
There was a problem hiding this comment.
Echoing what @clintropolis suggested, it would be useful to have a new dynamic config to toggle between the old and new implementations. We could keep the new config for a couple of releases which would be enough time for operators to test it out and later on we could do away with the config property.
Could we also consider including percentOfSegmentsToConsider as part of this new approach? This is a useful knob especially for larger clusters.
There was a problem hiding this comment.
It makes sense to me to retain the percentOfSegmentsToConsider and add an another dynamic parameter useBatchedSegmentSampler to enable the new improvement. In this way, the changing will be less aggressive.
There was a problem hiding this comment.
this doesn't really seem deprecated so much as no longer called at all, maybe we should remove it?
|
The new algorithm implementation looks good to me. But it drops the |
|
It would be useful to incorporate |
|
I haven't read the code yet, just reading comments and responding to my tag from @a2l007 ... I do think it is very helpful to keep the Also this patch and the issue it is resolving would help with the underlying performance issues of the design here. My patch to short circuit was more of a bandaid. This seems like a design improvement that visits the root issue of this method being invoked more than needed! So maybe my tuning config is less helpful once this PR is in? Or maybe it is still useful and both together would be great? |
Hi @capistrant, thanks for your comment. It's great to know that we both have tried to solve this issue. Ideally, this patch should be able to solve this issue fundamentally while it makes sense to me to evolve gradually in engineering. So, I agree to retain the And it'll be great if you can help to review this PR! |
Here's the benchmark data(or see JMH result graph) |
yuanlihan
force-pushed
the
reduce-method-invocation-of-reservoir-sampling
branch
from
6f2ad91 to
3b14ef2
Compare
yuanlihan
force-pushed
the
reduce-method-invocation-of-reservoir-sampling
branch
from
3b14ef2 to
b02a9e6
Compare
jihoonson
left a comment
jihoonson
left a comment
There was a problem hiding this comment.
@yuanlihan whoa, the benchmark result looks great 👍 The latest change LGTM.
|
|
||
| @Override | ||
| public BalancerSegmentHolder next() | ||
| { |
There was a problem hiding this comment.
nit:
if (!hasNext()) {
throw new NoSuchElementException();
}
|
@FrankChen021 @a2l007 @clintropolis do you have more comments? |
|
@yuanlihan Great work. Code change LGTM. For benchmark, could you add a combination of |
|
Thanks for this patch. Looking forward to using this in our clusters. It would be nice if you could add a few lines in the doc regarding a recommended value for |
asdf2014
left a comment
asdf2014
left a comment
There was a problem hiding this comment.
LGTM, thanks for your contribution
wow, awesome benchmark results! So sorry that I didn't participate more in the review of this PR, I hav been pulled away from Druid more than I'd like lately. Regardless, I see that y'all had it more than covered! So excited to start using this 💯 |
7 participants
Fixes #11256.
Description
Adding a new Reservoir Sampling method to sample K elements each time instead of only one element per method invocation.
A default method
pickSegmentsToMovewill be added to interfaceBalancerStrategyto pick K segments to move in a single method invocation.Key changed/added classes in this PR
BalancerStrategyReservoirSegmentSamplerBalanceSegmentsThis PR has: