You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/08/14 17:33:45 UTC
[GitHub] [druid] capistrant opened a new pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
capistrant opened a new pull request #10284:
URL: https://github.com/apache/druid/pull/10284
<!-- Thanks for trying to help us make Apache Druid be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
<!-- If you are a committer, follow the PR action item checklist for committers:
https://github.com/apache/druid/blob/master/dev/committer-instructions.md#pr-and-issue-action-item-checklist-for-committers. -->
### Description
<!-- Describe the goal of this PR, what problem are you fixing. If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
A large cluster with many segments results in a lot of work being done by the Coordinator in order to complete its duties. I believe that any optimization to coordinator duties can help in a large cluster. This patch gives an experienced admin a knob to turn in order to try and shave some time off of the balance segments duty. As of now, existing Balancer Strategies iterate over all of the segments in the cluster when choosing a segment to move. The first segment candidate is the most likely to be moved and the last segment candidate is the least likely to move. This patch gives an admin the ability to put a limit on the number of segments that will be candidates to be moved. For most cases, I don't think this knob will be needed, but in some large enterprise cases I feel that it could be beneficial.
<!-- Describe your patch: what did you change in code? How did you fix the problem? -->
I updated the BalancerStrategy Interface. The pickSegmentToMove method gained a 3rd parameter that specifies the number of segments that should be considered when picking a segment to move.
`CostBalancerStrategy` (and it's inheriting classes) and `RandomBalancerStrategy` both leverage `ReservoirSegmentSampler` to choose a segment "at random" from a list of candidate servers. I updated the required method in `ReservoirSegmentSampler` to adhere to the limiting parameter described above. If the limit is reached, the method picking a segment will break out of its iteration and return immediately.
Currently all code paths use a new dynamic coordinator config that an admin can tune if they'd like to put a limiter on this action of picking a segment to move. The default value for the config is such that all segments will be iterated and be candidates to pick. I thought a dynamic config was good because it is flexible and could be leveraged in times such as if you wanted to temporarily boost up the number of segments to move in order rebalance to new servers faster. If doing that, and you also wanted to make this go quicker by not bothering with having so many potential segments to be picked to move.
The new dynamic config is `maxSegmentsToConsiderPerMove` with a default of `Integer.MAX_VALUE`
I call out in the documentation that an admin should be experienced when considering altering this config. I say that because in many cases, the default is fine.
<!--
In each section, please describe design decisions made, including:
- Choice of algorithms
- Behavioral aspects. What configuration values are acceptable? How are corner cases and error conditions handled, such as when there are insufficient resources?
- Class organization and design (how the logic is split between classes, inheritance, composition, design patterns)
- Method organization and design (how the logic is split between methods, parameters and return types)
- Naming (class, method, API, configuration, HTTP endpoint, names of emitted metrics)
-->
<!-- It's good to describe an alternative design (or mention an alternative name) for every design (or naming) decision point and compare the alternatives with the designs that you've implemented (or the names you've chosen) to highlight the advantages of the chosen designs and names. -->
An alternative of this approach would be to restrict what is sent to `pickSegmentToMove` in the first place. I choose not to approach this at this time because I didn't like the idea of either choosing the number of `ServerHolders` to send to `pickSegmentsToMove` or to analyze the `ServerHolders` before picking how many to send to ensure only a certain number of segments are sent. I'd be open to re-assessing whether or not this would be a better approach or not if someone suggests it may be the proper approach.
<!-- If there was a discussion of the design of the feature implemented in this PR elsewhere (e. g. a "Proposal" issue, any other issue, or a thread in the development mailing list), link to that discussion from this PR description and explain what have changed in your final design compared to your original proposal or the consensus version in the end of the discussion. If something hasn't changed since the original discussion, you can omit a detailed discussion of those aspects of the design here, perhaps apart from brief mentioning for the sake of readability of this PR description. -->
<!-- Some of the aspects mentioned above may be omitted for simple and small changes. -->
<hr>
This PR has:
- [X] been self-reviewed.
- [X] added documentation for new or modified features or behaviors.
- [X] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [X] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [X] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [X] been tested in a test Druid cluster.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
<hr>
##### Key changed/added classes in this PR
* `BalancerStrategy` Interface
* `CostBalancerStrategy`
* `RandomBalancerStrategy`
* `ReservoirSegmentSampler`
* `CoordinatorDynamicConfig`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-748398266
@capistrant Sorry about not getting back to you on your last comment. If we set the percentage to a low enough value(1%) - could we verify that Druid never moves a segment because it never has any to consider. Not sure if this is possible, but I figured I'd throw this out there.
The test you added to `BalanceSegmentsTest` seems like a good step to protecting us from regressions here.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant edited a comment on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant edited a comment on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-721356289
> @capistrant Can we write integration tests for this dynamic config? Since it's not the default and used infrequently, I would hate for a refactoring to break this functionality and us not know about it
Good point, @suneet-s. I like the idea of protecting this against regressions, but I'm wondering if there is the proper plumbing in place to make this easy to test as is. Wondering if you have any inputs/ideas on how it may be accomplished.
We want to test the integration between the Coordinator's dynamic config and the act of balancing the cluster. Specifically, we want to ensure that our new config is being honored by Coordinator, meaning that balancing considers the expected % of segments when looking for segments to move.
The easiest way I can think of is that we have our integration tests cluster with a known number of segments in it. We set the config to some low % and monitor to make sure that no coordination cycle involves more moves than the number of segments in the cluster * (the config value / 100). ~~But what exactly would we monitor? The # of moves and segments let alone is logged by Druid currently. But do we really want to scrape logs? We could emit balancing stats and monitor the emitter. But not all balancer strategy implementations emit the proper fields, so we would need to modify that.~~ I guess `EmitClusterStatsAndMetrics` emit `segment/moved/count`. could we perform an integration test that monitors emitter output?
Do you have any ideas that I may be overlooking?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-674232612
> Would it make sense to perform the segment limiting for each `ServerHolder` individually rather than short circuiting the entire selection flow? Say if set `maxSegmentsToConsiderPerMove` to 50, it will consider only 50 segments per `ServerHolder` after which it will run the selection process for the next `ServerHolder`. That way, we can ensure that ReservoirSegmentSampler considers all the given servers before making the selection.
If the `ServerHolder` objects were passed in an indeterminate order, I would agree. But since they are not, I think that doing this could start to work against what I am trying to accomplish here. As an admin, I have determined that we are wasting our time bothering to consider segments once we have considered a certain amount of them. Because, after that point, the probability of choosing one of these segments is so low that I have deemed it to be not worth the resources used to consider it. My implementation says, if under normal balancing conditions (no limit), that all the segments beyond some point (probably) wouldn't be chosen anyways, let's just bail out early. Your suggestion here somewhat changes the fundamentals of the balancing strategy by now considering segments that would under normal balancing conditions, have had near zero chance of being chosen to now having a chance of being chosen, that I as an admin, have determined is likely enough to spend the resources to fi
nd out. And worse yet, those segments that may be chosen from `ServerHolders` at the end of the list are from Servers that I would probably prefer to be receiving segments rather than giving them up (in terms of consumption, not locality/performance), unless my cluster is very well balanced by consumption already.
Perhaps what we really want here is a way to configure the balancer to use one of many(?) implementations of "pick a segment from a list of `ServerHolder` objects". Instead of always using `ReservoirSegmentSampler#getRandomBalancerSegmentHolder` there could be multiple implementations of this action and an option for the admin to choose one with the current one being the default. I did slightly consider this in my head when I was thinking up the solution, but decided it may be too much of a heavy lift if the problem I am addressing is unique to certain large deployments who have tenants stressing over the execution time of a coordination cycle. Instead of combing up with a way to configure a selector and then add my slightly different selector with static coordinator configs instead of the dynamic config, I kind of wedged my implementation desire into the existing implementation.
> Also it might be more convenient for the user to express this parameter as a percentage of segments to select from rather than a number, but i guess that might take away some of the performance gain that we're trying to achieve here.
I had not considered this initially, but it is a good idea. I'd be okay with going this route if it is trivial to calculate the number of segments in the cluster including replication. I'll have to look at that.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant edited a comment on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant edited a comment on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-721356289
> @capistrant Can we write integration tests for this dynamic config? Since it's not the default and used infrequently, I would hate for a refactoring to break this functionality and us not know about it
Good point, @suneet-s. I like the idea of protecting this against regressions, but I'm wondering if there is the proper plumbing in place to make this easy to test as is. Wondering if you have any inputs/ideas on how it may be accomplished.
We want to test the integration between the Coordinator's dynamic config and the act of balancing the cluster. Specifically, we want to ensure that our new config is being honored by Coordinator, meaning that balancing considers the expected % of segments when looking for segments to move.
~~The easiest way I can think of is that we have our integration tests cluster with a known number of segments in it. We set the config to some low % and monitor to make sure that no coordination cycle involves more moves than the number of segments in the cluster * (the config value / 100). But what exactly would we monitor? The # of moves and segments let alone is logged by Druid currently. But do we really want to scrape logs? We could emit balancing stats and monitor the emitter. But not all balancer strategy implementations emit the proper fields, so we would need to modify that. I guess `EmitClusterStatsAndMetrics` emit `segment/moved/count`. could we perform an integration test that monitors emitter output?~~
The above section with strike through was me thinking of the wrong metric we are testing. we don't care about the number of segments moved. we care about the number of segments considered per move. back to the drawing board on ideas for this.
Do you have any ideas that I may be overlooking?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s merged pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
suneet-s merged pull request #10284:
URL: https://github.com/apache/druid/pull/10284
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a change in pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #10284:
URL: https://github.com/apache/druid/pull/10284#discussion_r546187069
##########
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:
ya, this is a good point. using double up front makes things a lot more straightforward
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10284:
URL: https://github.com/apache/druid/pull/10284#discussion_r546808822
##########
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:
makes sense to me
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-721208630
@capistrant Can we write integration tests for this dynamic config? Since it's not the default and used infrequently, I would hate for a refactoring to break this functionality and us not know about it
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 edited a comment on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
a2l007 edited a comment on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-674244483
> If the ServerHolder objects were passed in an indeterminate order, I would agree. But since they are not, I think that doing this could start to work against what I am trying to accomplish here. As an admin, I have determined that we are wasting our time bothering to consider segments once we have considered a certain amount of them. Because, after that point, the probability of choosing one of these segments is so low that I have deemed it to be not worth the resources used to consider it. My implementation says, if under normal balancing conditions (no limit), that all the segments beyond some point (probably) wouldn't be chosen anyways, let's just bail out early. Your suggestion here somewhat changes the fundamentals of the balancing strategy by now considering segments that would under normal balancing conditions, have had near zero chance of being chosen to now having a chance of being chosen, that I as an admin, have determined is likely enough to spend the resources to fi
nd out. And worse yet, those segments that may be chosen from ServerHolders at the end of the list are from Servers that I would probably prefer to be receiving segments rather than giving them up (in terms of consumption, not locality/performance), unless my cluster is very well balanced by consumption already.
Thanks for clarifying. My suggestion was basically ignoring the fact that the serverholder list is sorted by available space. In case we don't proceed to go the percentage route in this PR, it may be helpful to provide some recommendation in the docs on what might be a good value to set this to percentage-wise.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-674185232
I recently became a committer, but haven't gotten repo access setup yet (If another committer reads this and is willing to guide me on this process, I'm all ears!) so I cannot complete all of the committer steps. For labels, I would add:
* Design Review - It does add a config and change the balancing strategy interface so a design review seems necessary/smart
* Feature
* Performance (I did benchmark this in a dev cluster and we will see shaved time in the pickSegmentToMove code, but it will likely only be noticeable/worthwhile at very large scale)
* Area - Segment Balancing/Coordination
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on a change in pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10284:
URL: https://github.com/apache/druid/pull/10284#discussion_r546171041
##########
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.
EDIT: Something like
`Preconditions.checkArgument(percentOfSegmentsToConsiderPerMove > 0 && percentOfSegmentsToConsiderPerMove <= 100, "percentOfSegmentsToConsiderPerMove should be between 0 and 100!");`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-685992235
> > If the ServerHolder objects were passed in an indeterminate order, I would agree. But since they are not, I think that doing this could start to work against what I am trying to accomplish here. As an admin, I have determined that we are wasting our time bothering to consider segments once we have considered a certain amount of them. Because, after that point, the probability of choosing one of these segments is so low that I have deemed it to be not worth the resources used to consider it. My implementation says, if under normal balancing conditions (no limit), that all the segments beyond some point (probably) wouldn't be chosen anyways, let's just bail out early. Your suggestion here somewhat changes the fundamentals of the balancing strategy by now considering segments that would under normal balancing conditions, have had near zero chance of being chosen to now having a chance of being chosen, that I as an admin, have determined is likely enough to spend the resources to
find out. And worse yet, those segments that may be chosen from ServerHolders at the end of the list are from Servers that I would probably prefer to be receiving segments rather than giving them up (in terms of consumption, not locality/performance), unless my cluster is very well balanced by consumption already.
>
> Thanks for clarifying. My suggestion was basically ignoring the fact that the serverholder list is sorted by available space. In case we don't proceed to go the percentage route in this PR, it may be helpful to provide some recommendation in the docs on what might be a good value to set this to percentage-wise.
Sorry about accidentally editing your previous comment. Was not paying attention to what I was doing. I reverted the changes I made. On to what I was trying to say:
Finally circling back to this now that I have time... I decided to just update the doc for now. I am still trying to determine if the overhead to do percentages would require compute overhead that defeats the purpose of this. Looking at the code as I type this update.. `ServerHolder` contains an `ImmutableDruidServer` which has `int` instance variable `numSegments`. So I guess we could quickly get numSegments since it was essentially pre-computed for us. I worried that we'd have to stream the segments map to get the number on the fly. So all in, we'd iterate the ServerHolders first and get the number of segments and then calculate our `%` and use the same code with this new limiter. I do really like this as it is more flexible for long term durability. The raw number approach may require TLC when segment and server counts change significantly. I think we could chalk up the overhead to calculate the number of segments to consider as negligible. Say a cluster has `200 servers with 1
0k segments per server`. If we do `25%` we will add a loop that iterates a 200 item list and do a little math that will reduce the number of segment considerations from 2MM to 500k. We'd also skip the calculation if the value is 100% so still no added overhead for our default value users.
tl;dr I am going to implement the percentages approach instead of the raw number. The compute overhead to calculate the number of segments to iterate seems negligible compared to what we will save in the cases where this will be used (large clusters).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a change in pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #10284:
URL: https://github.com/apache/druid/pull/10284#discussion_r546187090
##########
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:
I'm cool with this being warn
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-679265465
> I recently became a committer, but haven't gotten repo access setup yet (If another committer reads this and is willing to guide me on this process, I'm all ears!) so I cannot complete all of the committer steps. For labels, I would add:
>
> * Design Review - It does add a config and change the balancing strategy interface so a design review seems necessary/smart
>
> * Feature
>
> * Performance (I did benchmark this in a dev cluster and we will see shaved time in the pickSegmentToMove code, but it will likely only be noticeable/worthwhile at very large scale)
>
> * Area - Segment Balancing/Coordination
Hey @capistrant, have you finished the setup for GitBox? You can check it here: https://gitbox.apache.org/setup/.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-748390512
@suneet-s This ended up being a hard one to come up with an integration test for since since it is hard to analyze what the code is doing and whether or not it is honoring this config. However, my latest commit adds a new test to BalanceSegmentsTest where it validates that pickSegmentToMove is called using the non-default value that the test uses for the new dynamic config. This provides some protection against regressions. What do you think about it?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] lgtm-com[bot] commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-686872650
This pull request **introduces 1 alert** when merging b3f680dfc1dcf0418b6c8cc8d910e2211779f10e into 3fc8bc0701938b532282b6b50398c0ee6503a517 - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-97f22426da44e4ea9b51748400e8e0a10bb01bdc)
**new alerts:**
* 1 for Missing space in string literal
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a change in pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #10284:
URL: https://github.com/apache/druid/pull/10284#discussion_r546805619
##########
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:
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
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 edited a comment on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
a2l007 edited a comment on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-674244483
Lucas
> If the ServerHolder objects were passed in an indeterminate order, I would agree. But since they are not, I think that doing this could start to work against what I am trying to accomplish here. As an admin, I have determined that we are wasting our time bothering to consider segments once we have considered a certain amount of them. Because, after that point, the probability of choosing one of these segments is so low that I have deemed it to be not worth the resources used to consider it. My implementation says, if under normal balancing conditions (no limit), that all the segments beyond some point (probably) wouldn't be chosen anyways, let's just bail out early. Your suggestion here somewhat changes the fundamentals of the balancing strategy by now considering segments that would under normal balancing conditions, have had near zero chance of being chosen to now having a chance of being chosen, that I as an admin, have determined is likely enough to spend the resources to fi
nd out. And worse yet, those segments that may be chosen from ServerHolders at the end of the list are from Servers that I would probably prefer to be receiving segments rather than giving them up (in terms of consumption, not locality/performance), unless my cluster is very well balanced by consumption already.
a2l007
> Thanks for clarifying. My suggestion was basically ignoring the fact that the serverholder list is sorted by available space. In case we don't proceed to go the percentage route in this PR, it may be helpful to provide some recommendation in the docs on what might be a good value to set this to percentage-wise.
Finally circling back to this now that I have time... I decided to just update the doc for now. I am still trying to determine if the overhead to do percentages would require compute overhead that defeats the purpose of this. Looking at the code as I type this update.. `ServerHolder` contains an `ImmutableDruidServer` which has `int` instance variable `numSegments`. So I guess we could quickly get numSegments since it was essentially pre-computed for us. I worried that we'd have to stream the segments map to get the number on the fly. So all in, we'd iterate the ServerHolders first and get the number of segments and then calculate our `%` and use the same code with this new limiter. I do really like this as it is more flexible for long term durability. The raw number approach may require TLC when segment and server counts change significantly. I think we could chalk up the overhead to calculate the number of segments to consider as negligible. Say a cluster has `200 servers with 1
0k segments per server`. If we do `25%` we will add a loop that iterates a 200 item list and do a little math that will reduce the number of segment considerations from 2MM to 500k. We'd also skip the calculation if the value is 100% so still no added overhead for our default value users.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
a2l007 commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-674208552
Would it make sense to perform the segment limiting for each `ServerHolder` individually rather than short circuiting the entire selection flow? Say if set `maxSegmentsToConsiderPerMove` to 50, it will consider only 50 segments per `ServerHolder` after which it will run the selection process for the next `ServerHolder`. That way, we can ensure that ReservoirSegmentSampler considers all the given servers before making the selection.
Also it might be more convenient for the user to express this parameter as a percentage of segments to select from rather than a number, but i guess that might take away some of the performance gain that we're trying to achieve here.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] lgtm-com[bot] commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-689065323
This pull request **introduces 1 alert** when merging a8a7c4277f4a1654a319bdf2f9668578cb260f05 into 176b7156249fdcd2a148c9d825f1f828df44709a - [view on LGTM.com](https://lgtm.com/projects/g/apache/druid/rev/pr-c6dad01c1b225042f6d99d5b89b8106bcb2f2f5c)
**new alerts:**
* 1 for Missing space in string literal
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
a2l007 commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-721204387
Could you please fix the spellcheck and console test errors as well?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
a2l007 commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-674244483
> If the ServerHolder objects were passed in an indeterminate order, I would agree. But since they are not, I think that doing this could start to work against what I am trying to accomplish here. As an admin, I have determined that we are wasting our time bothering to consider segments once we have considered a certain amount of them. Because, after that point, the probability of choosing one of these segments is so low that I have deemed it to be not worth the resources used to consider it. My implementation says, if under normal balancing conditions (no limit), that all the segments beyond some point (probably) wouldn't be chosen anyways, let's just bail out early. Your suggestion here somewhat changes the fundamentals of the balancing strategy by now considering segments that would under normal balancing conditions, have had near zero chance of being chosen to now having a chance of being chosen, that I as an admin, have determined is likely enough to spend the resources to fi
nd out. And worse yet, those segments that may be chosen from ServerHolders at the end of the list are from Servers that I would probably prefer to be receiving segments rather than giving them up (in terms of consumption, not locality/performance), unless my cluster is very well balanced by consumption already.
Thanks for clarifying. My suggestion was basically ignoring the fact that the serverholder list is sorted by available space. In case we don't proceed to go the percentage route in this PR, it may be helpful to provide some recommendation in the docs on what might be a good value to set this to percentage-wise.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on a change in pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #10284:
URL: https://github.com/apache/druid/pull/10284#discussion_r546187005
##########
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:
I agree, it's confusing to mask an error like I was. fixed
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] capistrant commented on pull request #10284: Add dynamic coordinator config that allows control over how many segments are considered when picking a segment to move.
Posted by GitBox <gi...@apache.org>.
capistrant commented on pull request #10284:
URL: https://github.com/apache/druid/pull/10284#issuecomment-721356289
> @capistrant Can we write integration tests for this dynamic config? Since it's not the default and used infrequently, I would hate for a refactoring to break this functionality and us not know about it
Good point, @suneet-s. I like the idea of protecting this against regressions, but I'm wondering if there is the proper plumbing in place to make this easy to test as is. Wondering if you have any inputs/ideas on how it may be accomplished.
We want to test the integration between the Coordinator's dynamic config and the act of balancing the cluster. Specifically, we want to ensure that our new config is being honored by Coordinator, meaning that balancing considers the expected % of segments when looking for segments to move.
The easiest way I can think of is that we have our integration tests cluster with a known number of segments in it. We set the config to some low % and monitor to make sure that no coordination cycle involves more moves than the number of segments in the cluster * (the config value / 100). But what exactly would we monitor? The # of moves and segments let alone is logged by Druid currently. But do we really want to scrape logs? We could emit balancing stats and monitor the emitter. But not all balancer strategy implementations emit the proper fields, so we would need to modify that.
Do you have any ideas that I may be overlooking?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org