-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move. #10284
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
Merged
Merged
Changes from all commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
614a96b
dynamic coord config adding more balancing control
f7c53cd
fix checkstyle failure
e1d4765
Make doc more detailed for admin to understand when/why to use new co…
f525f0a
Merge branch 'master' into bounded-segment-balancer
0ff7c5a
refactor PR to use a % of segments instead of raw number
f3993e3
update the docs
3b04213
remove bad doc line
d69897e
fix typo in name of new dynamic config
213599a
update RservoirSegmentSampler to gracefully deal with values > 100%
8e5a1df
add handler for <= 0 in ReservoirSegmentSampler
b3f680d
fixup CoordinatorDynamicConfigTest naming and argument ordering
a8a7c42
fix items in docs after spellcheck flags
fb20cb2
Fix lgtm flag on missing space in string literal
fd221c8
Merge branch 'master' into bounded-segment-balancer
85660be
improve documentation for new config
f899ade
Add default value to config docs and add advice in cluster tuning doc
848ac00
Add percentOfSegmentsToConsiderPerMove to web console coord config di…
982b016
update jest snapshot after console change
cce72eb
fix spell checker errors
1c8e9da
Improve debug logging in getRandomSegmentBalancerHolder to cover all …
48f0a92
Merge branch 'master' into bounded-segment-balancer
bf59074
add new config back to web console module after merge with master
dc3f80a
fix ReservoirSegmentSamplerTest
9ac54ff
fix line breaks in coordinator console dialog
9197bb0
Add a test that helps ensure not regressions for percentOfSegmentsToC…
6f24d44
Make improvements based off of feedback in review
62962e0
additional cleanup coming from review
93bed10
Add a warning log if limit on segments to consider for move can't be …
d76c924
remove unused import
e30ddf3
fix tests for CoordinatorDynamicConfig
43766ed
remove precondition test that is redundant in CoordinatorDynamicConfi…
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
|
|
||
| package org.apache.druid.server.coordinator; | ||
|
|
||
| import org.apache.druid.java.util.emitter.EmittingLogger; | ||
| import org.apache.druid.timeline.DataSegment; | ||
|
|
||
| import java.util.List; | ||
|
|
@@ -28,15 +29,54 @@ | |
| final class ReservoirSegmentSampler | ||
| { | ||
|
|
||
| private static final EmittingLogger log = new EmittingLogger(ReservoirSegmentSampler.class); | ||
|
|
||
| /** | ||
| * Iterates over segments that live on the candidate servers passed in {@link ServerHolder} and (possibly) picks a | ||
| * segment to return to caller in a {@link BalancerSegmentHolder} object. | ||
| * | ||
| * @param serverHolders List of {@link ServerHolder} objects containing segments who are candidates to be chosen. | ||
| * @param broadcastDatasources Set of DataSource names that identify broadcast datasources. We don't want to consider | ||
| * segments from these datasources. | ||
| * @param percentOfSegmentsToConsider The % of total cluster segments to consider before short-circuiting and | ||
| * returning immediately. | ||
| * @return | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the doc 👍 |
||
| static BalancerSegmentHolder getRandomBalancerSegmentHolder( | ||
| final List<ServerHolder> serverHolders, | ||
| Set<String> broadcastDatasources | ||
| Set<String> broadcastDatasources, | ||
| double percentOfSegmentsToConsider | ||
| ) | ||
| { | ||
| ServerHolder fromServerHolder = null; | ||
| DataSegment proposalSegment = null; | ||
| int calculatedSegmentLimit = Integer.MAX_VALUE; | ||
| int numSoFar = 0; | ||
|
|
||
| // Reset a bad value of percentOfSegmentsToConsider to 100. We don't allow consideration less than or equal to | ||
| // 0% of segments or greater than 100% of segments. | ||
| if (percentOfSegmentsToConsider <= 0 || percentOfSegmentsToConsider > 100) { | ||
| log.warn("Resetting percentOfSegmentsToConsider to 100 because only values from 1 to 100 are allowed." | ||
| + " You Provided [%f]", percentOfSegmentsToConsider); | ||
| percentOfSegmentsToConsider = 100; | ||
| } | ||
|
|
||
| // Calculate the integer limit for the number of segments to be considered for moving if % is less than 100 | ||
| if (percentOfSegmentsToConsider < 100) { | ||
| int totalSegments = 0; | ||
| for (ServerHolder server : serverHolders) { | ||
| totalSegments += server.getServer().getNumSegments(); | ||
| } | ||
| // If totalSegments are zero, we will assume it is a mistake and move on to iteration without updating | ||
| // calculatedSegmentLimit | ||
| if (totalSegments != 0) { | ||
| calculatedSegmentLimit = (int) Math.ceil((double) totalSegments * (percentOfSegmentsToConsider / 100.0)); | ||
| } else { | ||
| log.warn("Unable to calculate limit on segments to consider because ServerHolder collection indicates" | ||
| + " zero segments existing in the cluster."); | ||
| } | ||
| } | ||
|
|
||
| for (ServerHolder server : serverHolders) { | ||
| if (!server.getServer().getType().isSegmentReplicationTarget()) { | ||
| // if the server only handles broadcast segments (which don't need to be rebalanced), we have nothing to do | ||
|
|
@@ -56,6 +96,19 @@ static BalancerSegmentHolder getRandomBalancerSegmentHolder( | |
| proposalSegment = segment; | ||
| } | ||
| numSoFar++; | ||
|
|
||
| // We have iterated over the alloted number of segments and will return the currently proposed segment or null | ||
| // We will only break out early if we are iterating less than 100% of the total cluster segments | ||
| if (percentOfSegmentsToConsider < 100 && numSoFar >= calculatedSegmentLimit) { | ||
| log.debug("Breaking out of iteration over potential segments to move because we hit the limit [%f percent] of" | ||
| + " segments to consider to move. Segments Iterated: [%d]", percentOfSegmentsToConsider, numSoFar); | ||
| break; | ||
| } | ||
| } | ||
| // We have iterated over the alloted number of segments and will return the currently proposed segment or null | ||
| // We will only break out early if we are iterating less than 100% of the total cluster segments | ||
| if (percentOfSegmentsToConsider < 100 && numSoFar >= calculatedSegmentLimit) { | ||
| break; | ||
| } | ||
| } | ||
| if (fromServerHolder != null) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Should this have a PreCondition check that it falls between 0 - 100 ?
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.
hmm, Since this is a Builder class that builds a CoordinatorDynamicConfig object, I think this precondition is covered by the actual constructor for the CoordinatorDynamicConfig class. IMO, having another precondition here is redundant
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.
makes sense to me