clean up the balancing code around the batched vs deprecated way of sampling segments to balance #11960

capistrant · 2021-11-19T22:54:42Z

Start release notes suggestion

The dynamic coordinator config, percentOfSegmentsToConsiderPerMove has been deprecated and will be removed in a future release of Druid. It is being replaced by a new segment picking strategy introduced in #11257. This new strategy is currently toggled off by default, but can be toggled on if you set the dynamic coordinator config useBatchedSegmentSampler to true. Setting this as such, will disable the use of the deprecated percentOfSegmentsToConsiderPerMove. In a future release, useBatchedSegmentSampler will become permanently true.

End release notes suggestion

Description

Fixed unintended code path execution in BalancerStrategy#pickSegmentsToMove

#11257 Introduced a new way of sampling segments for balancing that is much more performant than the legacy way that exists today. There is a case in the code where the deprecated code can be executed despite, the dynamic config for batched segment sampling being enabled. In this line of code, if useBatchedSegmentSampler() == true and maxSegmentsToMove == 1, you would expect the batched segment sampler code to execute. However, you can see here that the new batched sampler is not used in this case because reservoir size is 1. This PR fixes this behavior by splitting BalancerStrategy#pickSegmentsToMove(List<ServerHolder>, Set<String>, int, double) into two methods using overloading - one for the new batched sampler, and one for the deprecated way of sampling one segment at a time. This change will also simplify cleanup of the deprecated code in what I assume will be the 0.24.X cycle.

Cleaned up the code for the competing segment pick strategies (deprecated vs batched segment sampler)

I did a few things to make the code more understandable and to make it abundantly clear that the old way of sampling segments to move with the help of the dynamic config percentOfSegmentsToConsiderPerMove is deprecated:

Added @Deprecated annotations to the CoordinatorDynamicConfig code related to percentOfSegmentsToConsiderPerMove
Updated the Coordinator Dynamic Config docs to explicitly state that percentOfSegmentsToConsiderPerMove is deprecated and will have no effect if useBatchedSegmentSampler dynamic config is true
Split BalancerStrategy#pickSegmentsToMove into two methods with the same name and overloaded arguments. One implementation is deprecated and takes a double for percentOfSegmentsToConsiderPerMove while the other is the new batched sampler which takes an int for the reservoir size.
- This change also makes BalanceSegments#balanceServers more understandable to those with limited familiarity with this work to migrate to the batched segment sampler. Before the call to BalancerStrategy#pickSegmentsToMove was opaque because it included both the reservoir size and the percent of segments to consider per move despite only one of those arguments being used depending on the value of the reservoir size (this also relates to the unintended code execution I talk about above). The new way makes it clear that if batched segment sampling is enabled, the non-deprecated pickSegmentsToMove is called. And if it is not, the deprecated pickSegmentsToMove is called.

Key changed/added classes in this PR

BalanceSegments
CoordinatorDynamicConfig
BalancerStrategy

This PR has:

been self-reviewed.
added documentation for new or modified features or behaviors.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
been tested in a test Druid cluster.

…ampling segments to balance

capistrant · 2021-11-19T22:55:33Z

@yuanlihan could you take a look at this since you wrote the original code for the batched sampler! I hope my code makes sense, but please comment on any mistakes I may have made!

yuanlihan · 2021-11-22T08:36:58Z

server/src/main/java/org/apache/druid/server/coordinator/duty/BalanceSegments.java

-    Iterator<BalancerSegmentHolder> segmentsToMove = strategy.pickSegmentsToMove(
-        toMoveFrom,
-        params.getBroadcastDatasources(),
-        params.getCoordinatorDynamicConfig().useBatchedSegmentSampler() ? maxSegmentsToMove : DEFAULT_RESERVOIR_SIZE,


I think we can delete this unused constant variable

yuanlihan · 2021-11-22T08:47:56Z

@yuanlihan could you take a look at this since you wrote the original code for the batched sampler! I hope my code makes sense, but please comment on any mistakes I may have made!

Hi @capistrant
It looks good to me. Thanks for making the code neat!

a2l007 · 2021-11-23T16:56:08Z

docs/configuration/index.md

@@ -886,7 +886,7 @@ Issuing a GET request at the same URL will return the spec that is currently in
 |`mergeSegmentsLimit`|The maximum number of segments that can be in a single [append task](../ingestion/tasks.md).|100|
 |`maxSegmentsToMove`|The maximum number of segments that can be moved at any given time.|5|
 |`useBatchedSegmentSampler`|Boolean flag for whether or not we should use the Reservoir Sampling with a reservoir of size k instead of fixed size 1 to pick segments to move. This option can be enabled to speed up segment balancing process, especially if there are huge number of segments in the cluster or if there are too many segments to move.|false|
-|`percentOfSegmentsToConsiderPerMove`|The percentage of the total number of segments in the cluster that are considered every time a segment needs to be selected for a move. Druid orders servers by available capacity ascending (the least available capacity first) and then iterates over the servers. For each server, Druid iterates over the segments on the server, considering them for moving. The default config of 100% means that every segment on every server is a candidate to be moved. This should make sense for most small to medium-sized clusters. However, an admin may find it preferable to drop this value lower if they don't think that it is worthwhile to consider every single segment in the cluster each time it is looking for a segment to move.|100|
+|`percentOfSegmentsToConsiderPerMove`|Deprecated. Will eventually be phased out by the batched segment sampler. Only used if `useBatchedSegmentSampler == false`. The percentage of the total number of segments in the cluster that are considered every time a segment needs to be selected for a move. Druid orders servers by available capacity ascending (the least available capacity first) and then iterates over the servers. For each server, Druid iterates over the segments on the server, considering them for moving. The default config of 100% means that every segment on every server is a candidate to be moved. This should make sense for most small to medium-sized clusters. However, an admin may find it preferable to drop this value lower if they don't think that it is worthwhile to consider every single segment in the cluster each time it is looking for a segment to move.|100|


Should we add a similar deprecation message in the web console description for this property?

yep, good call @a2l007

Can we document what the preferred alternative is here?

a2l007 · 2021-11-23T16:57:01Z

Tagging this under Release Notes so that users are aware of the deprecation of percentOfSegmentsToConsiderPerMove

…onsiderPerMove deprecated

capistrant · 2021-12-07T15:33:28Z

@a2l007 @yuanlihan thank you both for reviewing! I synced up with master and plan on merging once CI is green.

jihoonson · 2021-12-07T17:31:27Z

docs/configuration/index.md

@@ -886,7 +886,7 @@ Issuing a GET request at the same URL will return the spec that is currently in
 |`mergeSegmentsLimit`|The maximum number of segments that can be in a single [append task](../ingestion/tasks.md).|100|
 |`maxSegmentsToMove`|The maximum number of segments that can be moved at any given time.|5|
 |`useBatchedSegmentSampler`|Boolean flag for whether or not we should use the Reservoir Sampling with a reservoir of size k instead of fixed size 1 to pick segments to move. This option can be enabled to speed up segment balancing process, especially if there are huge number of segments in the cluster or if there are too many segments to move.|false|
-|`percentOfSegmentsToConsiderPerMove`|The percentage of the total number of segments in the cluster that are considered every time a segment needs to be selected for a move. Druid orders servers by available capacity ascending (the least available capacity first) and then iterates over the servers. For each server, Druid iterates over the segments on the server, considering them for moving. The default config of 100% means that every segment on every server is a candidate to be moved. This should make sense for most small to medium-sized clusters. However, an admin may find it preferable to drop this value lower if they don't think that it is worthwhile to consider every single segment in the cluster each time it is looking for a segment to move.|100|
+|`percentOfSegmentsToConsiderPerMove`|Deprecated. Will eventually be phased out by the batched segment sampler. Only used if `useBatchedSegmentSampler == false`. The percentage of the total number of segments in the cluster that are considered every time a segment needs to be selected for a move. Druid orders servers by available capacity ascending (the least available capacity first) and then iterates over the servers. For each server, Druid iterates over the segments on the server, considering them for moving. The default config of 100% means that every segment on every server is a candidate to be moved. This should make sense for most small to medium-sized clusters. However, an admin may find it preferable to drop this value lower if they don't think that it is worthwhile to consider every single segment in the cluster each time it is looking for a segment to move.|100|


Can we document what the preferred alternative is here?

jihoonson · 2021-12-07T17:31:57Z

web-console/src/druid-models/coordinator-dynamic-config.tsx

-        This should make sense for most small to medium-sized clusters. However, an admin may find
-        it preferable to drop this value lower if they don&apos;t think that it is worthwhile to
-        consider every single segment in the cluster each time it is looking for a segment to move.
+        Deprecated. This will be phased out by the batched segment sampler. Only used if


Can we document what the preferred alternative is here as well?

could you clarify what you mean by this? I take it as you asking me to state that the preferred alternative is to set useBatchedSegmentSampler == true. so perhaps:

Deprecated. Will eventually be phased out by the batched segment sampler. You can enable the batched segment sampler now by setting the dynamic Coordinator config, useBatchedSegmentSampler, to true. Note that if you choose enable the batched segment sampler, this config has no effect on balancing. If useBatchedSegmentSampler == false, this config defines the percentage of the total number of segments in the cluster that are considered every time a segment needs to be selected for a move. Druid orders servers by available capacity ascending (the least available capacity first) and then iterates over the servers. For each server, Druid iterates over the segments on the server, considering them for moving. The default config of 100% means that every segment on every server is a candidate to be moved. This should make sense for most small to medium-sized clusters. However, an admin may find it preferable to drop this value lower if they don't think that it is worthwhile to consider every single segment in the cluster each time it is looking for a segment to move.|100|

Sorry for ambiguity. Yes, encouraging using useBatchedSegmentSampler instead will be enough. The one in your comment looks good to me.

jihoonson

LGTM. Please consider my comments before merging it.

jihoonson · 2021-12-07T19:28:00Z

@capistrant thank you for updating docs. Feel free to merge once the "docs" CI passes.

jihoonson · 2021-12-07T22:47:42Z

The "docs' CI passed. Merging this PR.

clean up the balancing code around the batched vs deprecated way of s…

0927377

…ampling segments to balance

capistrant added the Area - Segment Balancing/Coordination label Nov 19, 2021

yuanlihan reviewed Nov 22, 2021

View reviewed changes

capistrant added 3 commits November 22, 2021 12:17

fix docs, clarify comments, add deprecated annotations to legacy code

44a66bb

remove unused variable

755b316

Merge branch 'master' into batch-segment-sampler-fix

4804810

a2l007 reviewed Nov 23, 2021

View reviewed changes

a2l007 added the Release Notes label Nov 23, 2021

capistrant added 4 commits November 24, 2021 06:58

update dynamic config dialog in console to state percentOfSegmentsToC…

92b532b

…onsiderPerMove deprecated

fix dynamic config text for percentOfSegmentsToConsiderPerMove

1aaecfd

run prettier to cleanup coordinator-dynamic-config.tsx changes

3a18f27

update jest snapshot

3ca6b1e

a2l007 approved these changes Nov 30, 2021

View reviewed changes

Merge branch 'master' into batch-segment-sampler-fix

d3e2675

jihoonson reviewed Dec 7, 2021

View reviewed changes

jihoonson approved these changes Dec 7, 2021

View reviewed changes

update documentation per review feedback

acfd534

jihoonson merged commit 150902b into apache:master Dec 7, 2021

abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022

abhishekagarwal87 mentioned this pull request Jun 22, 2022

[Draft] 0.23.0 Release notes #12510

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean up the balancing code around the batched vs deprecated way of sampling segments to balance #11960

clean up the balancing code around the batched vs deprecated way of sampling segments to balance #11960

capistrant commented Nov 19, 2021 •

edited

capistrant commented Nov 19, 2021

yuanlihan Nov 22, 2021

yuanlihan commented Nov 22, 2021

a2l007 Nov 23, 2021

capistrant Nov 24, 2021

jihoonson Dec 7, 2021

a2l007 commented Nov 23, 2021

capistrant commented Dec 7, 2021

jihoonson Dec 7, 2021

jihoonson Dec 7, 2021

capistrant Dec 7, 2021 •

edited

jihoonson Dec 7, 2021

jihoonson left a comment

jihoonson commented Dec 7, 2021

jihoonson commented Dec 7, 2021

clean up the balancing code around the batched vs deprecated way of sampling segments to balance #11960

clean up the balancing code around the batched vs deprecated way of sampling segments to balance #11960

Conversation

capistrant commented Nov 19, 2021 • edited

Description

Fixed unintended code path execution in BalancerStrategy#pickSegmentsToMove

Cleaned up the code for the competing segment pick strategies (deprecated vs batched segment sampler)

Key changed/added classes in this PR

capistrant commented Nov 19, 2021

yuanlihan Nov 22, 2021

Choose a reason for hiding this comment

yuanlihan commented Nov 22, 2021

a2l007 Nov 23, 2021

Choose a reason for hiding this comment

capistrant Nov 24, 2021

Choose a reason for hiding this comment

jihoonson Dec 7, 2021

Choose a reason for hiding this comment

a2l007 commented Nov 23, 2021

capistrant commented Dec 7, 2021

jihoonson Dec 7, 2021

Choose a reason for hiding this comment

jihoonson Dec 7, 2021

Choose a reason for hiding this comment

capistrant Dec 7, 2021 • edited

Choose a reason for hiding this comment

jihoonson Dec 7, 2021

Choose a reason for hiding this comment

jihoonson left a comment

Choose a reason for hiding this comment

jihoonson commented Dec 7, 2021

jihoonson commented Dec 7, 2021

capistrant commented Nov 19, 2021 •

edited

capistrant Dec 7, 2021 •

edited