suneet-s commented on a change in pull request #10284:
URL: https://github.com/apache/druid/pull/10284#discussion_r546171183
##########
File path:
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java
##########
@@ -95,6 +96,7 @@ public CoordinatorDynamicConfig(
@JsonProperty("mergeBytesLimit") long mergeBytesLimit,
@JsonProperty("mergeSegmentsLimit") int mergeSegmentsLimit,
@JsonProperty("maxSegmentsToMove") int maxSegmentsToMove,
+ @JsonProperty("percentOfSegmentsToConsiderPerMove") int
percentOfSegmentsToConsiderPerMove,
Review comment:
Should this be a double since we're dealing with percentages? I think it
would make it less likely for someone in the future to make some math mistake
by accidentally dividing by an integer.
##########
File path:
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java
##########
@@ -123,6 +125,12 @@ public CoordinatorDynamicConfig(
this.mergeBytesLimit = mergeBytesLimit;
this.mergeSegmentsLimit = mergeSegmentsLimit;
this.maxSegmentsToMove = maxSegmentsToMove;
+ // This helps with ease of migration to the new config, but could confuse
users. Docs explicitly state this value must be > 0
+ if (percentOfSegmentsToConsiderPerMove <= 0 ||
percentOfSegmentsToConsiderPerMove > 100) {
+ this.percentOfSegmentsToConsiderPerMove = 100;
Review comment:
Since this is a json configured property, I think we should throw an
exception here so the user knows they've done something incorrect.
##########
File path:
server/src/main/java/org/apache/druid/server/coordinator/CoordinatorDynamicConfig.java
##########
@@ -463,6 +485,7 @@ public Builder(
this.mergeBytesLimit = mergeBytesLimit;
this.mergeSegmentsLimit = mergeSegmentsLimit;
this.maxSegmentsToMove = maxSegmentsToMove;
+ this.percentOfSegmentsToConsiderPerMove =
percentOfSegmentsToConsiderPerMove;
Review comment:
Should this have a PreCondition check that it falls between 0 - 100 ?
##########
File path:
server/src/main/java/org/apache/druid/server/coordinator/ReservoirSegmentSampler.java
##########
@@ -28,15 +29,51 @@
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
+ */
static BalancerSegmentHolder getRandomBalancerSegmentHolder(
final List<ServerHolder> serverHolders,
- Set<String> broadcastDatasources
+ Set<String> broadcastDatasources,
+ int 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.debug("Resetting percentOfSegmentsToConsider to 100 because only
values from 1 to 100 are allowed."
Review comment:
IMO this should be at least a WARN since it should be impossible that
`percentOfSegmentsToConsider` should have already been checked earlier in the
system
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
