You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/03/30 18:07:18 UTC
[GitHub] [pinot] jackjlli opened a new pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
jackjlli opened a new pull request #8441:
URL: https://github.com/apache/pinot/pull/8441
## Description
This PR adds `retainInstancesSequence` feature to table rebalance to minimize data movement between instances within the pool.
Related issue: https://github.com/apache/pinot/issues/8371
E.g. if the old instances are: [I1, I2, I3, I4] and the new instances become: [I1, I3, I4, I5, I6], the removed instance is I2 and the newly added instances are I5 and I6.
The position of I2 would be replaced by I5, the new remaining I6 would be appended to the tail.
Thus, the new order would be [I1, I5, I3, I4, I6].
The copy of instancesPartitions would be kept in the ZNRecord if this feature is enabled, so that this copy can be reused for the next calculation.
## Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
* [ ] Yes (Please label as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR fix a zero-downtime upgrade introduced earlier?
* [ ] Yes (Please label this as **<code>backward-incompat</code>**, and complete the section below on Release Notes)
Does this PR otherwise need attention when creating release notes? Things to consider:
- New configuration options
- Deprecation of configurations
- Signature changes to public methods/interfaces
- New plugins added or old plugins removed
* [ ] Yes (Please label this PR as **<code>release-notes</code>** and complete the section on Release Notes)
## Release Notes
<!-- If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release. -->
<!-- If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text.
-->
## Documentation
<!-- If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document
-->
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8455118) into [master](https://codecov.io/gh/apache/pinot/commit/91c9ce4aebca3913065e08263c6e80f7f515dd50?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91c9ce4) will **decrease** coverage by `0.02%`.
> The diff coverage is `69.79%`.
> :exclamation: Current head 8455118 differs from pull request most recent head 87b949a. Consider uploading reports for the commit 87b949a to get more accurate results
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
============================================
- Coverage 64.21% 64.18% -0.03%
+ Complexity 4282 4280 -2
============================================
Files 1618 1619 +1
Lines 85376 85510 +134
Branches 13004 13033 +29
============================================
+ Hits 54820 54883 +63
- Misses 26576 26636 +60
- Partials 3980 3991 +11
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `67.00% <35.59%> (-0.05%)` | :arrow_down: |
| unittests2 | `14.21% <58.85%> (+0.05%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...g/apache/pinot/core/operator/DocIdSetOperator.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9Eb2NJZFNldE9wZXJhdG9yLmphdmE=) | `92.85% <ø> (-0.25%)` | :arrow_down: |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `75.76% <20.00%> (-1.45%)` | :arrow_down: |
| [...ources/PinotInstanceAssignmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90SW5zdGFuY2VBc3NpZ25tZW50UmVzdGxldFJlc291cmNlLmphdmE=) | `71.21% <71.42%> (-2.02%)` | :arrow_down: |
| [...not/core/query/reduce/GroupByDataTableReducer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvR3JvdXBCeURhdGFUYWJsZVJlZHVjZXIuamF2YQ==) | `87.41% <73.91%> (+0.88%)` | :arrow_up: |
| [...nce/RetainedSequenceInstanceConstraintApplier.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvUmV0YWluZWRTZXF1ZW5jZUluc3RhbmNlQ29uc3RyYWludEFwcGxpZXIuamF2YQ==) | `92.85% <92.85%> (ø)` | |
| [.../assignment/instance/InstanceAssignmentDriver.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VBc3NpZ25tZW50RHJpdmVyLmphdmE=) | `97.82% <95.83%> (+1.82%)` | :arrow_up: |
| [...e/assignment/instance/InstanceTagPoolSelector.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VUYWdQb29sU2VsZWN0b3IuamF2YQ==) | `96.07% <96.72%> (-0.22%)` | :arrow_down: |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `44.48% <100.00%> (+0.19%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `64.22% <100.00%> (ø)` | |
| ... and [13 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [91c9ce4...87b949a](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1086320784
> > > I don't follow why we need to store the pool to instances mapping in the ZK. Essentially what we need is to pass the existing instance partitions to the `InstanceReplicaGroupPartitionSelector.selectInstances()`, and add a flag to enable `minimizeMovement` and use a greedy algorithm to retain as much instances as possible from the candidate instances from each pool.
> >
> >
> > Yes the initial plan is to reuse the existing instance partitions, but the thing is that for the instance which no longer exists in the current list of instance configs (which is gone), there is no way to know which pool it came from; i.e. even though we know there is an empty seat there, we don't know which pool we should select the new instance from. As long as we can control the input parameters of `replicaPartitionSelector.selectInstances()` (the input instancePartitions is always empty, thus as long as we control the instance sequence of `poolToInstanceConfigsMap`), we can always get the deterministic instancePartitions as output.
>
> Actually we can know the pool where the original instance is coming from without storing the pool information. The pool selection is deterministic, and is based on the hash code of the table name. Following the same algorithm, for each replica id, you can get the same pool id as the original assignment. Then you may run the greedy algorithm to pick the instances from the candidate instances from the pool to achieve minimum movement.
IMO there is no hard requirement for a pool id to be mapped ~~1:1~~ 1:N to a replica id, right? It's just in current strategy of `InstanceReplicaGroupPartitionSelector` we assign instances from a pool to a replica. But this should not be enforced for future use, especially right now we are implementing selector with FD awareness and it can have instances from multiple pools in 1 replica group.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1085241390
I don't follow why we need to store the pool to instances mapping in the ZK. Essentially what we need is to pass the existing instance partitions to the `InstanceReplicaGroupPartitionSelector.selectInstances()`, and add a flag to enable `minimizeMovement` and use a greedy algorithm to retain as much instances as possible from the candidate instances from each pool.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840945946
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -77,7 +95,31 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
InstancePartitions instancePartitions = new InstancePartitions(
instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the pool to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPoolToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
Review comment:
Good question! Although there are two added configs, they actually are not in the same method. `retainInstanceSequence` is in the caller method of `InstanceAssignmentDriver.assignInstances()`.
`shouldRetainInstanceSequence` is inside the method of `InstanceAssignmentDriver.assignInstances()`.
these two configs denotes the same thing, i.e. whether to denote whether this feature should be enabled or not.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840888705
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -77,7 +95,31 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
InstancePartitions instancePartitions = new InstancePartitions(
instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the pool to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPoolToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
Review comment:
IMO we should only persist the instance sequence if it's required.
The actual value of `shouldRetainInstanceSequence` is determined by `(existingPoolToInstancesMap != null)`. There are two cases need to be differentiated:
* If it's false, that means `retainInstanceSequence` is set to false in the rebalance config. There is no need to retain instance sequence.
* If it's true but the map is empty, that means `retainInstanceSequence` is set to true but there is no such map in the ZK yet.
Inside the driver, if `retainInstanceSequence` from rebalance config is true, the existingPoolToInstancesMap should be fetched from ZK. The first fetch would return empty map since there is no such map persisted in the ZK yet. But since `shouldRetainInstanceSequence` is true because `(existingPoolToInstancesMap != null)`, this new map will be persisted into the ZK for the next calculation. So next time, when the same request comes, this map can be reused.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840888705
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -77,7 +95,31 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
InstancePartitions instancePartitions = new InstancePartitions(
instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the pool to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPoolToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
Review comment:
IMO we should only persist the instance sequence if it's required.
The actual value of `shouldRetainInstanceSequence` is determined by `(existingPoolToInstancesMap != null)`. There are two cases need to differentiate:
* If it's false, that means `retainInstanceSequence` is set to false in the rebalance config. There is no need to retain instance sequence.
* If it's true but the map is empty, that means `retainInstanceSequence` is set to true but there is no such map in the ZK yet.
Inside the driver, if `retainInstanceSequence` from rebalance config is true, the existingPoolToInstancesMap should be fetched from ZK. The first fetch would return empty map since there is no such map persisted in the ZK yet. But since `shouldRetainInstanceSequence` is true because `(existingPoolToInstancesMap != null)`, this new map will be persisted into the ZK for the next calculation. So next time, when the same request comes, this map can be reused.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840969840
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +52,117 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param existingPoolToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> existingPoolToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // If existingPoolToInstancesMap is null, treat it as an empty map.
+ if (existingPoolToInstancesMap == null) {
+ existingPoolToInstancesMap = Collections.emptyMap();
+ }
+ // Filter out the instances with the correct tag.
+ // Use LinkedHashMap here to retain the sorted list of instance names.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ // A deque is used here in order to retain the sequence,
+ // given the fact that the list of instance configs is always sorted.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : existingPoolToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ Map<Integer, List<InstanceConfig>> poolToLatestInstanceConfigsMap = new TreeMap<>();
if (_tagPoolConfig.isPoolBased()) {
- // Pool based selection
+ // Pool based selection. All the instances should be associated with a specific pool number.
+ // Instance selection should be done within the same pool.
+ // E.g.: Pool0 -> [ I1, I2, I3 ]
+ // Pool1 -> [ I4, I5, I6 ]
- // Extract the pool information from the instance configs
- for (InstanceConfig instanceConfig : candidateInstanceConfigs) {
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
+ // Each pool number associates with a list of newly added instance configs,
+ // so that new instances can be fetched from this list.
+ Map<Integer, Deque<InstanceConfig>> poolToNewInstanceConfigsMap = new HashMap<>();
+
+ // Extract the pool information from the instance configs.
+ for (Map.Entry<String, InstanceConfig> entry : candidateInstanceConfigsMap.entrySet()) {
+ String instanceName = entry.getKey();
+ InstanceConfig instanceConfig = entry.getValue();
Map<String, String> poolMap = instanceConfig.getRecord().getMapField(InstanceUtils.POOL_KEY);
if (poolMap != null && poolMap.containsKey(tag)) {
int pool = Integer.parseInt(poolMap.get(tag));
- poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new ArrayList<>()).add(instanceConfig);
+ poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new TreeMap<>()).put(instanceName, instanceConfig);
+ if (newlyAddedInstances.contains(instanceName)) {
+ poolToNewInstanceConfigsMap.computeIfAbsent(pool, k -> new LinkedList<>()).add(instanceConfig);
+ }
+ }
+ }
+
+ for (Map.Entry<Integer, List<String>> entry : existingPoolToInstancesMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<String> existingInstanceAssignmentInPool = entry.getValue();
+ List<InstanceConfig> candidateInstanceConfigsWithSequence = new ArrayList<>();
+ for (String existingInstance: existingInstanceAssignmentInPool) {
+ InstanceConfig instanceConfig = poolToInstanceConfigsMap.get(pool).get(existingInstance);
+ // Add instances to the candidate list and respect the sequence of the existing instances from the ZK.
+ // The missing/removed instances will be replaced by the newly instances.
+ // If the instance still exists from ZK, then add it to the candidate list.
+ // E.g. if the old instances are: [I1, I2, I3, I4] and the new instance are: [I1, I3, I4, I5, I6],
+ // the removed instance is I2 and the newly added instances are I5 and I6.
+ // The position of I2 would be replaced by I5, the new remaining I6 would be appended to the tail.
+ // Thus, the new order would be [I1, I5, I3, I4, I6].
Review comment:
The rotation is applied only when it's a new pool. In order to retain the existing instance sequence, we shouldn't rotate the list any more, even if the number of pool has changed.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a8dec88) into [master](https://codecov.io/gh/apache/pinot/commit/91c9ce4aebca3913065e08263c6e80f7f515dd50?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91c9ce4) will **increase** coverage by `5.13%`.
> The diff coverage is `68.51%`.
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
============================================
+ Coverage 64.21% 69.34% +5.13%
- Complexity 4282 4283 +1
============================================
Files 1618 1664 +46
Lines 85376 87433 +2057
Branches 13004 13243 +239
============================================
+ Hits 54820 60634 +5814
+ Misses 26576 22536 -4040
- Partials 3980 4263 +283
```
| Flag | Coverage Δ | |
|---|---|---|
| integration2 | `25.98% <0.61%> (?)` | |
| unittests1 | `67.03% <0.00%> (-0.02%)` | :arrow_down: |
| unittests2 | `14.25% <67.90%> (+0.09%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `23.43% <3.12%> (+23.43%)` | :arrow_up: |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `75.76% <20.00%> (-1.45%)` | :arrow_down: |
| [...ources/PinotInstanceAssignmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90SW5zdGFuY2VBc3NpZ25tZW50UmVzdGxldFJlc291cmNlLmphdmE=) | `70.99% <63.63%> (-2.24%)` | :arrow_down: |
| [...nce/RetainedSequenceInstanceConstraintApplier.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvUmV0YWluZWRTZXF1ZW5jZUluc3RhbmNlQ29uc3RyYWludEFwcGxpZXIuamF2YQ==) | `92.85% <92.85%> (ø)` | |
| [.../assignment/instance/InstanceAssignmentDriver.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VBc3NpZ25tZW50RHJpdmVyLmphdmE=) | `97.82% <95.83%> (+1.82%)` | :arrow_up: |
| [...e/assignment/instance/InstanceTagPoolSelector.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VUYWdQb29sU2VsZWN0b3IuamF2YQ==) | `96.07% <96.72%> (-0.22%)` | :arrow_down: |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `50.34% <100.00%> (+6.05%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `67.72% <100.00%> (+3.50%)` | :arrow_up: |
| [...ance/HashBasedRotateInstanceConstraintApplier.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSGFzaEJhc2VkUm90YXRlSW5zdGFuY2VDb25zdHJhaW50QXBwbGllci5qYXZh) | `93.33% <100.00%> (+1.02%)` | :arrow_up: |
| [...he/pinot/segment/local/segment/store/IndexKey.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3N0b3JlL0luZGV4S2V5LmphdmE=) | `65.00% <0.00%> (-10.00%)` | :arrow_down: |
| ... and [346 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [91c9ce4...a8dec88](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839051143
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -51,16 +52,34 @@ public InstanceAssignmentDriver(TableConfig tableConfig) {
_tableConfig = tableConfig;
}
+ /**
+ * Assign instances to InstancePartitions object.
+ * @param instancePartitionsType type of instance partitions
+ * @param instanceConfigs list of instance configs
+ * @param partitionToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
+ */
public InstancePartitions assignInstances(InstancePartitionsType instancePartitionsType,
- List<InstanceConfig> instanceConfigs) {
+ List<InstanceConfig> instanceConfigs, Map<Integer, List<String>> partitionToInstancesMap) {
+ boolean shouldRetainInstanceSequence = !partitionToInstancesMap.isEmpty();
String tableNameWithType = _tableConfig.getTableName();
- LOGGER.info("Starting {} instance assignment for table: {}", instancePartitionsType, tableNameWithType);
+ LOGGER.info("Starting {} instance assignment for table: {}. Should retain instance sequence: {}",
+ instancePartitionsType, tableNameWithType, shouldRetainInstanceSequence);
InstanceAssignmentConfig assignmentConfig =
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(_tableConfig, instancePartitionsType);
InstanceTagPoolSelector tagPoolSelector =
new InstanceTagPoolSelector(assignmentConfig.getTagPoolConfig(), tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = tagPoolSelector.selectInstances(instanceConfigs);
+ Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap =
+ tagPoolSelector.selectInstances(instanceConfigs, partitionToInstancesMap);
+
+ InstancePartitions instancePartitions = new InstancePartitions(
+ instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the partition to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPartitionToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
InstanceConstraintConfig constraintConfig = assignmentConfig.getConstraintConfig();
Review comment:
Related question -- should we create a new strategy for this to simplify code and avoid adding multiple conditions to existing code ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839047121
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -75,9 +94,26 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
InstanceReplicaGroupPartitionSelector replicaPartitionSelector =
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
- InstancePartitions instancePartitions = new InstancePartitions(
- instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
replicaPartitionSelector.selectInstances(poolToInstanceConfigsMap, instancePartitions);
return instancePartitions;
}
+
+ private Map<Integer, List<String>> extractInstanceNamesFromPoolToInstanceConfigsMap(
+ Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap) {
+ Map<Integer, List<String>> partitionToInstancesMap = new TreeMap<>();
+ for (Map.Entry<Integer, List<InstanceConfig>> entry : poolToInstanceConfigsMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<InstanceConfig> instanceConfigs = entry.getValue();
+ partitionToInstancesMap.put(pool, extractInstanceNamesFromInstanceConfigs(instanceConfigs));
Review comment:
This is confusing -- poolID is not the same as partitionID
It is `partitionToInstancesMap` but you are using the key as `pool` which does not sound correct
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8455118) into [master](https://codecov.io/gh/apache/pinot/commit/91c9ce4aebca3913065e08263c6e80f7f515dd50?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91c9ce4) will **increase** coverage by `6.45%`.
> The diff coverage is `72.91%`.
> :exclamation: Current head 8455118 differs from pull request most recent head 87b949a. Consider uploading reports for the commit 87b949a to get more accurate results
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
============================================
+ Coverage 64.21% 70.66% +6.45%
+ Complexity 4282 4280 -2
============================================
Files 1618 1664 +46
Lines 85376 87417 +2041
Branches 13004 13237 +233
============================================
+ Hits 54820 61769 +6949
+ Misses 26576 21349 -5227
- Partials 3980 4299 +319
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `27.03% <10.93%> (?)` | |
| integration2 | `25.84% <8.85%> (?)` | |
| unittests1 | `67.00% <35.59%> (-0.05%)` | :arrow_down: |
| unittests2 | `14.21% <58.85%> (+0.05%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...g/apache/pinot/core/operator/DocIdSetOperator.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9Eb2NJZFNldE9wZXJhdG9yLmphdmE=) | `92.85% <ø> (-0.25%)` | :arrow_down: |
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `23.43% <3.12%> (+23.43%)` | :arrow_up: |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `77.71% <33.33%> (+0.50%)` | :arrow_up: |
| [...ources/PinotInstanceAssignmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90SW5zdGFuY2VBc3NpZ25tZW50UmVzdGxldFJlc291cmNlLmphdmE=) | `71.21% <71.42%> (-2.02%)` | :arrow_down: |
| [...not/core/query/reduce/GroupByDataTableReducer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvR3JvdXBCeURhdGFUYWJsZVJlZHVjZXIuamF2YQ==) | `90.55% <86.95%> (+4.03%)` | :arrow_up: |
| [...nce/RetainedSequenceInstanceConstraintApplier.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvUmV0YWluZWRTZXF1ZW5jZUluc3RhbmNlQ29uc3RyYWludEFwcGxpZXIuamF2YQ==) | `92.85% <92.85%> (ø)` | |
| [.../assignment/instance/InstanceAssignmentDriver.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VBc3NpZ25tZW50RHJpdmVyLmphdmE=) | `97.82% <95.83%> (+1.82%)` | :arrow_up: |
| [...e/assignment/instance/InstanceTagPoolSelector.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VUYWdQb29sU2VsZWN0b3IuamF2YQ==) | `96.07% <96.72%> (-0.22%)` | :arrow_down: |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `50.34% <100.00%> (+6.05%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `67.66% <100.00%> (+3.44%)` | :arrow_up: |
| ... and [382 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [91c9ce4...87b949a](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo commented on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1086320784
> > > I don't follow why we need to store the pool to instances mapping in the ZK. Essentially what we need is to pass the existing instance partitions to the `InstanceReplicaGroupPartitionSelector.selectInstances()`, and add a flag to enable `minimizeMovement` and use a greedy algorithm to retain as much instances as possible from the candidate instances from each pool.
> >
> >
> > Yes the initial plan is to reuse the existing instance partitions, but the thing is that for the instance which no longer exists in the current list of instance configs (which is gone), there is no way to know which pool it came from; i.e. even though we know there is an empty seat there, we don't know which pool we should select the new instance from. As long as we can control the input parameters of `replicaPartitionSelector.selectInstances()` (the input instancePartitions is always empty, thus as long as we control the instance sequence of `poolToInstanceConfigsMap`), we can always get the deterministic instancePartitions as output.
>
> Actually we can know the pool where the original instance is coming from without storing the pool information. The pool selection is deterministic, and is based on the hash code of the table name. Following the same algorithm, for each replica id, you can get the same pool id as the original assignment. Then you may run the greedy algorithm to pick the instances from the candidate instances from the pool to achieve minimum movement.
IMO there is no hard requirement for a pool id to be mapped 1:1 to a replica id, right? It's just in current strategy of `InstanceReplicaGroupPartitionSelector` we assign instances from a pool to a replica. But this should not be enforced for future use, especially right now we are implementing selector with FD awareness and it can have instances from multiple pools in 1 replica group.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839303449
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -51,16 +52,34 @@ public InstanceAssignmentDriver(TableConfig tableConfig) {
_tableConfig = tableConfig;
}
+ /**
+ * Assign instances to InstancePartitions object.
+ * @param instancePartitionsType type of instance partitions
+ * @param instanceConfigs list of instance configs
+ * @param partitionToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
+ */
public InstancePartitions assignInstances(InstancePartitionsType instancePartitionsType,
- List<InstanceConfig> instanceConfigs) {
+ List<InstanceConfig> instanceConfigs, Map<Integer, List<String>> partitionToInstancesMap) {
+ boolean shouldRetainInstanceSequence = !partitionToInstancesMap.isEmpty();
String tableNameWithType = _tableConfig.getTableName();
- LOGGER.info("Starting {} instance assignment for table: {}", instancePartitionsType, tableNameWithType);
+ LOGGER.info("Starting {} instance assignment for table: {}. Should retain instance sequence: {}",
+ instancePartitionsType, tableNameWithType, shouldRetainInstanceSequence);
InstanceAssignmentConfig assignmentConfig =
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(_tableConfig, instancePartitionsType);
InstanceTagPoolSelector tagPoolSelector =
new InstanceTagPoolSelector(assignmentConfig.getTagPoolConfig(), tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = tagPoolSelector.selectInstances(instanceConfigs);
+ Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap =
+ tagPoolSelector.selectInstances(instanceConfigs, partitionToInstancesMap);
+
+ InstancePartitions instancePartitions = new InstancePartitions(
+ instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the partition to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPartitionToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
InstanceConstraintConfig constraintConfig = assignmentConfig.getConstraintConfig();
Review comment:
Yes I did think of extending the `InstanceConstraintApplier`, but I do want to keep the current logic if the `existingPoolToInstancesMap` is not given. Plus, it'd be better to store the instance with the sequence before the rotation, as it will always return the deterministic list of instance if the input instance sequence is fixed. And I don't like the idea of overriding the existing method as it'd be hard to maintain two copies of code if some changes is needed.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839050322
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitions.java
##########
@@ -58,25 +60,37 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class InstancePartitions {
private static final char PARTITION_REPLICA_GROUP_SEPARATOR = '_';
+ private static final String PARTITIONS_KEY = "partitions";
+ private static final String INSTANCE_SEPARATOR = "/";
private final String _instancePartitionsName;
- private final Map<String, List<String>> _partitionToInstancesMap;
+ private final Map<String, List<String>> _partitionWithReplicaGroupToInstancesMap;
+ private final Map<Integer, List<String>> _partitionToInstancesMap;
Review comment:
Is the key in `_partitionToInstancesMap` the partitionID ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839286532
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -75,9 +94,26 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
InstanceReplicaGroupPartitionSelector replicaPartitionSelector =
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
- InstancePartitions instancePartitions = new InstancePartitions(
- instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
replicaPartitionSelector.selectInstances(poolToInstanceConfigsMap, instancePartitions);
return instancePartitions;
}
+
+ private Map<Integer, List<String>> extractInstanceNamesFromPoolToInstanceConfigsMap(
+ Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap) {
+ Map<Integer, List<String>> partitionToInstancesMap = new TreeMap<>();
+ for (Map.Entry<Integer, List<InstanceConfig>> entry : poolToInstanceConfigsMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<InstanceConfig> instanceConfigs = entry.getValue();
+ partitionToInstancesMap.put(pool, extractInstanceNamesFromInstanceConfigs(instanceConfigs));
Review comment:
Good catch! Updated the variable name to `poolToInstanceConfigsMap` as the key denotes the pool number.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jtao15 commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840958651
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +52,121 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param existingPoolToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> existingPoolToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // If existingPoolToInstancesMap is null, treat it as an empty map.
+ if (existingPoolToInstancesMap == null) {
+ existingPoolToInstancesMap = Collections.emptyMap();
+ }
+ // Filter out the instances with the correct tag.
+ // Use LinkedHashMap here to retain the sorted list of instance names.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ // A deque is used here in order to retain the sequence,
+ // given the fact that the list of instance configs is always sorted.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : existingPoolToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ Map<Integer, List<InstanceConfig>> poolToLatestInstanceConfigsMap = new TreeMap<>();
if (_tagPoolConfig.isPoolBased()) {
- // Pool based selection
+ // Pool based selection. All the instances should be associated with a specific pool number.
+ // Instance selection should be done within the same pool.
+ // E.g.: Pool0 -> [ I1, I2, I3 ]
+ // Pool1 -> [ I4, I5, I6 ]
- // Extract the pool information from the instance configs
- for (InstanceConfig instanceConfig : candidateInstanceConfigs) {
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
+ // Each pool number associates with a list of newly added instance configs,
+ // so that new instances can be fetched from this list.
+ Map<Integer, Deque<InstanceConfig>> poolToNewInstanceConfigsMap = new HashMap<>();
+
+ // Extract the pool information from the instance configs.
+ for (Map.Entry<String, InstanceConfig> entry : candidateInstanceConfigsMap.entrySet()) {
+ String instanceName = entry.getKey();
+ InstanceConfig instanceConfig = entry.getValue();
Map<String, String> poolMap = instanceConfig.getRecord().getMapField(InstanceUtils.POOL_KEY);
if (poolMap != null && poolMap.containsKey(tag)) {
int pool = Integer.parseInt(poolMap.get(tag));
- poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new ArrayList<>()).add(instanceConfig);
+ poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new TreeMap<>()).put(instanceName, instanceConfig);
+ if (newlyAddedInstances.contains(instanceName)) {
+ poolToNewInstanceConfigsMap.computeIfAbsent(pool, k -> new LinkedList<>()).add(instanceConfig);
+ }
+ }
+ }
+
+ for (Map.Entry<Integer, List<String>> entry : existingPoolToInstancesMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<String> existingInstanceAssignmentInPool = entry.getValue();
+ List<InstanceConfig> candidateInstanceConfigsWithSequence = new ArrayList<>();
+ for (String existingInstance: existingInstanceAssignmentInPool) {
+ InstanceConfig instanceConfig = poolToInstanceConfigsMap.get(pool).get(existingInstance);
+ // Add instances to the candidate list and respect the sequence of the existing instances from the ZK.
+ // The missing/removed instances will be replaced by the newly instances.
+ // If the instance still exists from ZK, then add it to the candidate list.
+ // E.g. if the old instances are: [I1, I2, I3, I4] and the new instance are: [I1, I3, I4, I5, I6],
+ // the removed instance is I2 and the newly added instances are I5 and I6.
+ // The position of I2 would be replaced by I5, the new remaining I6 would be appended to the tail.
+ // Thus, the new order would be [I1, I5, I3, I4, I6].
+ if (instanceConfig != null) {
+ candidateInstanceConfigsWithSequence.add(instanceConfig);
+ } else {
+ // The current chosen instance no longer lives in the cluster any more, thus pick a new instance.
+ InstanceConfig newInstanceConfig = poolToNewInstanceConfigsMap.get(pool).pollFirst();
+ // If there is no new instance from the same pool, then don't add it.
+ if (newInstanceConfig != null) {
+ candidateInstanceConfigsWithSequence.add(newInstanceConfig);
+ }
+ }
}
+ poolToLatestInstanceConfigsMap.put(pool, candidateInstanceConfigsWithSequence);
Review comment:
`candidateInstanceConfigsWithSequence` can be empty if the pool does not exist any more (both `poolToInstanceConfigsMap.get(pool)` and `poolToNewInstanceConfigsMap.get(pool)` are null)?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +52,117 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param existingPoolToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> existingPoolToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // If existingPoolToInstancesMap is null, treat it as an empty map.
+ if (existingPoolToInstancesMap == null) {
+ existingPoolToInstancesMap = Collections.emptyMap();
+ }
+ // Filter out the instances with the correct tag.
+ // Use LinkedHashMap here to retain the sorted list of instance names.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ // A deque is used here in order to retain the sequence,
+ // given the fact that the list of instance configs is always sorted.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : existingPoolToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ Map<Integer, List<InstanceConfig>> poolToLatestInstanceConfigsMap = new TreeMap<>();
if (_tagPoolConfig.isPoolBased()) {
- // Pool based selection
+ // Pool based selection. All the instances should be associated with a specific pool number.
+ // Instance selection should be done within the same pool.
+ // E.g.: Pool0 -> [ I1, I2, I3 ]
+ // Pool1 -> [ I4, I5, I6 ]
- // Extract the pool information from the instance configs
- for (InstanceConfig instanceConfig : candidateInstanceConfigs) {
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
+ // Each pool number associates with a list of newly added instance configs,
+ // so that new instances can be fetched from this list.
+ Map<Integer, Deque<InstanceConfig>> poolToNewInstanceConfigsMap = new HashMap<>();
+
+ // Extract the pool information from the instance configs.
+ for (Map.Entry<String, InstanceConfig> entry : candidateInstanceConfigsMap.entrySet()) {
+ String instanceName = entry.getKey();
+ InstanceConfig instanceConfig = entry.getValue();
Map<String, String> poolMap = instanceConfig.getRecord().getMapField(InstanceUtils.POOL_KEY);
if (poolMap != null && poolMap.containsKey(tag)) {
int pool = Integer.parseInt(poolMap.get(tag));
- poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new ArrayList<>()).add(instanceConfig);
+ poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new TreeMap<>()).put(instanceName, instanceConfig);
+ if (newlyAddedInstances.contains(instanceName)) {
+ poolToNewInstanceConfigsMap.computeIfAbsent(pool, k -> new LinkedList<>()).add(instanceConfig);
+ }
+ }
+ }
+
+ for (Map.Entry<Integer, List<String>> entry : existingPoolToInstancesMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<String> existingInstanceAssignmentInPool = entry.getValue();
+ List<InstanceConfig> candidateInstanceConfigsWithSequence = new ArrayList<>();
+ for (String existingInstance: existingInstanceAssignmentInPool) {
+ InstanceConfig instanceConfig = poolToInstanceConfigsMap.get(pool).get(existingInstance);
+ // Add instances to the candidate list and respect the sequence of the existing instances from the ZK.
+ // The missing/removed instances will be replaced by the newly instances.
+ // If the instance still exists from ZK, then add it to the candidate list.
+ // E.g. if the old instances are: [I1, I2, I3, I4] and the new instance are: [I1, I3, I4, I5, I6],
+ // the removed instance is I2 and the newly added instances are I5 and I6.
+ // The position of I2 would be replaced by I5, the new remaining I6 would be appended to the tail.
+ // Thus, the new order would be [I1, I5, I3, I4, I6].
Review comment:
`Collections.rotate(instanceConfigs, -(tableNameHash % numInstanceConfigs));`
This does not guarantee the same rotation because `numInstanceConfigs` is different?
Also, even if we maintain the order after we apply the constraints, current `InstanceReplicaGroupPartitionSelector` will pick instances based on # of replica-groups for each pool.
Say we want to change from 2* 2 to 3* 2, the old instance sequence is [I1, I2, I3, I4] and the new sequence is [I1, I5, I3, I4, I6, I7]. The old assignment is:
```
r1 -> {I1, I3}
r2 -> {I2, I4}
```
The new assignment will be:
```
r1 -> {I1, I4}
r2 -> {I5, I6}
r3 -> {I3, I7}
```
But ideally I3 and I4 should not be moved from one replica to another.
We need to improve InstanceReplicaGroupPartitionSelector.selectInstances() to handle such case.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1084240228
@jasperjiaguo you might also want to review this PR considering you are changing similar code for another purpose
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] Jackie-Jiang commented on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
Jackie-Jiang commented on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1086291495
> > I don't follow why we need to store the pool to instances mapping in the ZK. Essentially what we need is to pass the existing instance partitions to the `InstanceReplicaGroupPartitionSelector.selectInstances()`, and add a flag to enable `minimizeMovement` and use a greedy algorithm to retain as much instances as possible from the candidate instances from each pool.
>
> Yes the initial plan is to reuse the existing instance partitions, but the thing is that for the instance which no longer exists in the current list of instance configs (which is gone), there is no way to know which pool it came from; i.e. even though we know there is an empty seat there, we don't know which pool we should select the new instance from. As long as we can control the input parameters of `replicaPartitionSelector.selectInstances()` (the input instancePartitions is always empty, thus as long as we control the instance sequence of `poolToInstanceConfigsMap`), we can always get the deterministic instancePartitions as output.
Actually we can know the pool where the original instance is coming from without storing the pool information. The pool selection is deterministic, and is based on the hash code of the table name. Following the same algorithm, for each replica id, you can get the same pool id as the original assignment. Then you may run the greedy algorithm to pick the instances from the candidate instances from the pool to achieve minimum movement.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840196170
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java
##########
@@ -145,6 +145,8 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
tableConfig.getRoutingConfig().getInstanceSelectorType());
boolean bestEfforts = rebalanceConfig.getBoolean(RebalanceConfigConstants.BEST_EFFORTS,
RebalanceConfigConstants.DEFAULT_BEST_EFFORTS);
+ boolean retainInstanceSequence = rebalanceConfig.getBoolean(RebalanceConfigConstants.RETAIN_INSTANCE_SEQUENCE,
+ RebalanceConfigConstants.DEFAULT_RETAIN_INSTANCE_SEQUENCE);
Review comment:
`retainInstanceSequence` being true only works when `reassignInstances` is true, because if `reassignInstances` is false, there is actually no need to reassign any instances, let alone minimizing data movement.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jtao15 commented on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jtao15 commented on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1086359754
> > > > I don't follow why we need to store the pool to instances mapping in the ZK. Essentially what we need is to pass the existing instance partitions to the `InstanceReplicaGroupPartitionSelector.selectInstances()`, and add a flag to enable `minimizeMovement` and use a greedy algorithm to retain as much instances as possible from the candidate instances from each pool.
> > >
> > >
> > > Yes the initial plan is to reuse the existing instance partitions, but the thing is that for the instance which no longer exists in the current list of instance configs (which is gone), there is no way to know which pool it came from; i.e. even though we know there is an empty seat there, we don't know which pool we should select the new instance from. As long as we can control the input parameters of `replicaPartitionSelector.selectInstances()` (the input instancePartitions is always empty, thus as long as we control the instance sequence of `poolToInstanceConfigsMap`), we can always get the deterministic instancePartitions as output.
> >
> >
> > Actually we can know the pool where the original instance is coming from without storing the pool information. The pool selection is deterministic, and is based on the hash code of the table name. Following the same algorithm, for each replica id, you can get the same pool id as the original assignment. Then you may run the greedy algorithm to pick the instances from the candidate instances from the pool to achieve minimum movement.
>
> IMO there is no hard requirement for a pool id to be mapped 1:1 to a replica id, right? It's just in current strategy of `InstanceReplicaGroupPartitionSelector` we assign instances from a pool to a replica. But this should not be enforced for future use, especially right now we are implementing selector with FD awareness and it can have instances from multiple pools in 1 replica group.
If we want to achieve minimum segments movement, we should guarantee the pool number is not changed for a replica group before and after the rebalance, else we are moving one replica from a set of hosts to another. The pool selection will be deterministic if the number of pools remains the same. Maybe we can set the scope to only handle the case that the number of pools didn't change.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1086320784
> > > I don't follow why we need to store the pool to instances mapping in the ZK. Essentially what we need is to pass the existing instance partitions to the `InstanceReplicaGroupPartitionSelector.selectInstances()`, and add a flag to enable `minimizeMovement` and use a greedy algorithm to retain as much instances as possible from the candidate instances from each pool.
> >
> >
> > Yes the initial plan is to reuse the existing instance partitions, but the thing is that for the instance which no longer exists in the current list of instance configs (which is gone), there is no way to know which pool it came from; i.e. even though we know there is an empty seat there, we don't know which pool we should select the new instance from. As long as we can control the input parameters of `replicaPartitionSelector.selectInstances()` (the input instancePartitions is always empty, thus as long as we control the instance sequence of `poolToInstanceConfigsMap`), we can always get the deterministic instancePartitions as output.
>
> Actually we can know the pool where the original instance is coming from without storing the pool information. The pool selection is deterministic, and is based on the hash code of the table name. Following the same algorithm, for each replica id, you can get the same pool id as the original assignment. Then you may run the greedy algorithm to pick the instances from the candidate instances from the pool to achieve minimum movement.
IMO there is no hard requirement for a pool id to be mapped ~~1:1~~ 1:N to a replica id, right? It's just in current strategy of `InstanceReplicaGroupPartitionSelector` we assign instances from a pool to a replica. But this should not be enforced for future use, especially right now we are implementing selector with FD awareness and it can have instances from multiple pools in 1 replica group.
In other words we should not rely on the status-quo of we can "reverse engineering the pool id from replica group id". So I think the pool -> instance mapping should probably be saved.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840819278
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -121,12 +190,40 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
LOGGER.info("Selecting pools: {} for table: {}", poolsToSelect, _tableNameWithType);
pools.retainAll(poolsToSelect);
} else {
- // Non-pool based selection
+ // Non-pool based selection. All the instances should be associated with a single pool, which is always 0.
+ // E.g.: Pool0 -> [ I1, I2, I3, I4, I5, I6 ]
- LOGGER.info("Selecting {} instances for table: {}", numCandidateInstances, _tableNameWithType);
+ LOGGER.info("Selecting {} instances for table: {}", candidateInstanceConfigsMap.size(), _tableNameWithType);
// Put all instance configs as pool 0
- poolToInstanceConfigsMap.put(0, candidateInstanceConfigs);
+
+ for (Map.Entry<Integer, List<String>> entry : existingPoolToInstancesMap.entrySet()) {
Review comment:
If the existing pool doesn't have any instances any more, we shouldn't add it to the candidate list.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840924957
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -77,7 +95,31 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
InstancePartitions instancePartitions = new InstancePartitions(
instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the pool to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPoolToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
Review comment:
I'm wondering would it be better if we can have one config instead of two to avoid some confusion. I mean as a user I might expect the `retainInstanceSequence` in rebalance to retain current sequence, whereas it's actually a "best effort" depending on if the corresponding flag is set in the initial assignment.
Similarly for current large use-cases without this map already available we might need some migration plan?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8455118) into [master](https://codecov.io/gh/apache/pinot/commit/91c9ce4aebca3913065e08263c6e80f7f515dd50?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91c9ce4) will **increase** coverage by `5.35%`.
> The diff coverage is `72.91%`.
> :exclamation: Current head 8455118 differs from pull request most recent head 87b949a. Consider uploading reports for the commit 87b949a to get more accurate results
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
============================================
+ Coverage 64.21% 69.56% +5.35%
+ Complexity 4282 4280 -2
============================================
Files 1618 1664 +46
Lines 85376 87417 +2041
Branches 13004 13237 +233
============================================
+ Hits 54820 60816 +5996
+ Misses 26576 22324 -4252
- Partials 3980 4277 +297
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `27.03% <10.93%> (?)` | |
| unittests1 | `67.00% <35.59%> (-0.05%)` | :arrow_down: |
| unittests2 | `14.21% <58.85%> (+0.05%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...g/apache/pinot/core/operator/DocIdSetOperator.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9Eb2NJZFNldE9wZXJhdG9yLmphdmE=) | `92.85% <ø> (-0.25%)` | :arrow_down: |
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `23.43% <3.12%> (+23.43%)` | :arrow_up: |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `77.71% <33.33%> (+0.50%)` | :arrow_up: |
| [...ources/PinotInstanceAssignmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90SW5zdGFuY2VBc3NpZ25tZW50UmVzdGxldFJlc291cmNlLmphdmE=) | `71.21% <71.42%> (-2.02%)` | :arrow_down: |
| [...not/core/query/reduce/GroupByDataTableReducer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvR3JvdXBCeURhdGFUYWJsZVJlZHVjZXIuamF2YQ==) | `90.55% <86.95%> (+4.03%)` | :arrow_up: |
| [...nce/RetainedSequenceInstanceConstraintApplier.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvUmV0YWluZWRTZXF1ZW5jZUluc3RhbmNlQ29uc3RyYWludEFwcGxpZXIuamF2YQ==) | `92.85% <92.85%> (ø)` | |
| [.../assignment/instance/InstanceAssignmentDriver.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VBc3NpZ25tZW50RHJpdmVyLmphdmE=) | `97.82% <95.83%> (+1.82%)` | :arrow_up: |
| [...e/assignment/instance/InstanceTagPoolSelector.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VUYWdQb29sU2VsZWN0b3IuamF2YQ==) | `96.07% <96.72%> (-0.22%)` | :arrow_down: |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `48.62% <100.00%> (+4.33%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `67.46% <100.00%> (+3.24%)` | :arrow_up: |
| ... and [341 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [91c9ce4...87b949a](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8455118) into [master](https://codecov.io/gh/apache/pinot/commit/91c9ce4aebca3913065e08263c6e80f7f515dd50?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91c9ce4) will **increase** coverage by `2.79%`.
> The diff coverage is `35.59%`.
> :exclamation: Current head 8455118 differs from pull request most recent head 87b949a. Consider uploading reports for the commit 87b949a to get more accurate results
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
============================================
+ Coverage 64.21% 67.00% +2.79%
+ Complexity 4282 4196 -86
============================================
Files 1618 1261 -357
Lines 85376 63765 -21611
Branches 13004 10015 -2989
============================================
- Hits 54820 42725 -12095
+ Misses 26576 17964 -8612
+ Partials 3980 3076 -904
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests1 | `67.00% <35.59%> (-0.05%)` | :arrow_down: |
| unittests2 | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...g/apache/pinot/core/operator/DocIdSetOperator.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9Eb2NJZFNldE9wZXJhdG9yLmphdmE=) | `92.85% <ø> (-0.25%)` | :arrow_down: |
| [...not/core/query/reduce/GroupByDataTableReducer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9yZWR1Y2UvR3JvdXBCeURhdGFUYWJsZVJlZHVjZXIuamF2YQ==) | `87.41% <73.91%> (+0.88%)` | :arrow_up: |
| [...org/apache/pinot/core/util/trace/TraceContext.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL3RyYWNlL1RyYWNlQ29udGV4dC5qYXZh) | `73.58% <100.00%> (ø)` | |
| [...ment/creator/impl/SegmentColumnarIndexCreator.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L2NyZWF0b3IvaW1wbC9TZWdtZW50Q29sdW1uYXJJbmRleENyZWF0b3IuamF2YQ==) | `86.57% <100.00%> (+0.07%)` | :arrow_up: |
| [...he/pinot/segment/local/segment/store/IndexKey.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3N0b3JlL0luZGV4S2V5LmphdmE=) | `65.00% <0.00%> (-10.00%)` | :arrow_down: |
| [.../impl/dictionary/BaseOffHeapMutableDictionary.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL2RpY3Rpb25hcnkvQmFzZU9mZkhlYXBNdXRhYmxlRGljdGlvbmFyeS5qYXZh) | `81.33% <0.00%> (-3.34%)` | :arrow_down: |
| [.../pinot/core/query/scheduler/PriorityScheduler.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9zY2hlZHVsZXIvUHJpb3JpdHlTY2hlZHVsZXIuamF2YQ==) | `80.82% <0.00%> (-2.74%)` | :arrow_down: |
| [...org/apache/pinot/core/data/table/TableResizer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1RhYmxlUmVzaXplci5qYXZh) | `91.30% <0.00%> (-1.09%)` | :arrow_down: |
| [...core/query/executor/ServerQueryExecutorV1Impl.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9leGVjdXRvci9TZXJ2ZXJRdWVyeUV4ZWN1dG9yVjFJbXBsLmphdmE=) | `60.09% <0.00%> (-0.46%)` | :arrow_down: |
| ... and [358 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [91c9ce4...87b949a](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840976672
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +52,117 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param existingPoolToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> existingPoolToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // If existingPoolToInstancesMap is null, treat it as an empty map.
+ if (existingPoolToInstancesMap == null) {
+ existingPoolToInstancesMap = Collections.emptyMap();
+ }
+ // Filter out the instances with the correct tag.
+ // Use LinkedHashMap here to retain the sorted list of instance names.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ // A deque is used here in order to retain the sequence,
+ // given the fact that the list of instance configs is always sorted.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : existingPoolToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ Map<Integer, List<InstanceConfig>> poolToLatestInstanceConfigsMap = new TreeMap<>();
if (_tagPoolConfig.isPoolBased()) {
- // Pool based selection
+ // Pool based selection. All the instances should be associated with a specific pool number.
+ // Instance selection should be done within the same pool.
+ // E.g.: Pool0 -> [ I1, I2, I3 ]
+ // Pool1 -> [ I4, I5, I6 ]
- // Extract the pool information from the instance configs
- for (InstanceConfig instanceConfig : candidateInstanceConfigs) {
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
+ // Each pool number associates with a list of newly added instance configs,
+ // so that new instances can be fetched from this list.
+ Map<Integer, Deque<InstanceConfig>> poolToNewInstanceConfigsMap = new HashMap<>();
+
+ // Extract the pool information from the instance configs.
+ for (Map.Entry<String, InstanceConfig> entry : candidateInstanceConfigsMap.entrySet()) {
+ String instanceName = entry.getKey();
+ InstanceConfig instanceConfig = entry.getValue();
Map<String, String> poolMap = instanceConfig.getRecord().getMapField(InstanceUtils.POOL_KEY);
if (poolMap != null && poolMap.containsKey(tag)) {
int pool = Integer.parseInt(poolMap.get(tag));
- poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new ArrayList<>()).add(instanceConfig);
+ poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new TreeMap<>()).put(instanceName, instanceConfig);
+ if (newlyAddedInstances.contains(instanceName)) {
+ poolToNewInstanceConfigsMap.computeIfAbsent(pool, k -> new LinkedList<>()).add(instanceConfig);
+ }
+ }
+ }
+
+ for (Map.Entry<Integer, List<String>> entry : existingPoolToInstancesMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<String> existingInstanceAssignmentInPool = entry.getValue();
+ List<InstanceConfig> candidateInstanceConfigsWithSequence = new ArrayList<>();
+ for (String existingInstance: existingInstanceAssignmentInPool) {
+ InstanceConfig instanceConfig = poolToInstanceConfigsMap.get(pool).get(existingInstance);
+ // Add instances to the candidate list and respect the sequence of the existing instances from the ZK.
+ // The missing/removed instances will be replaced by the newly instances.
+ // If the instance still exists from ZK, then add it to the candidate list.
+ // E.g. if the old instances are: [I1, I2, I3, I4] and the new instance are: [I1, I3, I4, I5, I6],
+ // the removed instance is I2 and the newly added instances are I5 and I6.
+ // The position of I2 would be replaced by I5, the new remaining I6 would be appended to the tail.
+ // Thus, the new order would be [I1, I5, I3, I4, I6].
Review comment:
Regarding to your 2nd question, it's a mixed scenario of swapping a host and adding 1 more replica group.
For swapping a host, the current algorithm works well as I5 is going to replace I2 as expected.
For adding 1 more replica group, yes the current algorithm will inevitably shuffle the instances for different replica groups. We can add a TODO here and solve it in another PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839066546
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +51,108 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param partitionToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> partitionToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // Filter out the instances with the correct tag.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : partitionToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
Review comment:
`InstanceConfig` already has the `InstanceName`. Why do we need to store name as key ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839047121
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -75,9 +94,26 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
InstanceReplicaGroupPartitionSelector replicaPartitionSelector =
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
- InstancePartitions instancePartitions = new InstancePartitions(
- instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
replicaPartitionSelector.selectInstances(poolToInstanceConfigsMap, instancePartitions);
return instancePartitions;
}
+
+ private Map<Integer, List<String>> extractInstanceNamesFromPoolToInstanceConfigsMap(
+ Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap) {
+ Map<Integer, List<String>> partitionToInstancesMap = new TreeMap<>();
+ for (Map.Entry<Integer, List<InstanceConfig>> entry : poolToInstanceConfigsMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<InstanceConfig> instanceConfigs = entry.getValue();
+ partitionToInstancesMap.put(pool, extractInstanceNamesFromInstanceConfigs(instanceConfigs));
Review comment:
This is confusing -- poolID is not the same as partitionID
It is `partitionToInstancesMap` but you are using the key as `pool`
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839295778
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +51,108 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param partitionToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> partitionToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // Filter out the instances with the correct tag.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : partitionToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
Review comment:
The purpose of storing instanceName as the key of the inner map is fast retrieve the instance config given the instance name in a pool. Say in Pool 0, I want to get the instance config given a new instance name from `newlyAddedInstances`, then I can fetch it in O(1) time and assign it to the target pool.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839292161
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitions.java
##########
@@ -58,25 +60,37 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class InstancePartitions {
private static final char PARTITION_REPLICA_GROUP_SEPARATOR = '_';
+ private static final String PARTITIONS_KEY = "partitions";
+ private static final String INSTANCE_SEPARATOR = "/";
private final String _instancePartitionsName;
- private final Map<String, List<String>> _partitionToInstancesMap;
+ private final Map<String, List<String>> _partitionWithReplicaGroupToInstancesMap;
+ private final Map<Integer, List<String>> _partitionToInstancesMap;
Review comment:
I did think about reusing the existing InstancePartitions from ZNode, but the problem is that there is no way to know about the pool information of an instance if its instance config is gone from the cluster; we don't know which pool the gone instance belongs to, so we don't know which pool we should pick the new instance from.
On the other hand, since we can choose which table should we enable this feature, the overhead on the ZNode should be controllable.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839022456
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -51,16 +52,34 @@ public InstanceAssignmentDriver(TableConfig tableConfig) {
_tableConfig = tableConfig;
}
+ /**
+ * Assign instances to InstancePartitions object.
+ * @param instancePartitionsType type of instance partitions
+ * @param instanceConfigs list of instance configs
+ * @param partitionToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
+ */
public InstancePartitions assignInstances(InstancePartitionsType instancePartitionsType,
- List<InstanceConfig> instanceConfigs) {
+ List<InstanceConfig> instanceConfigs, Map<Integer, List<String>> partitionToInstancesMap) {
+ boolean shouldRetainInstanceSequence = !partitionToInstancesMap.isEmpty();
String tableNameWithType = _tableConfig.getTableName();
- LOGGER.info("Starting {} instance assignment for table: {}", instancePartitionsType, tableNameWithType);
+ LOGGER.info("Starting {} instance assignment for table: {}. Should retain instance sequence: {}",
+ instancePartitionsType, tableNameWithType, shouldRetainInstanceSequence);
InstanceAssignmentConfig assignmentConfig =
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(_tableConfig, instancePartitionsType);
InstanceTagPoolSelector tagPoolSelector =
new InstanceTagPoolSelector(assignmentConfig.getTagPoolConfig(), tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = tagPoolSelector.selectInstances(instanceConfigs);
+ Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap =
+ tagPoolSelector.selectInstances(instanceConfigs, partitionToInstancesMap);
+
+ InstancePartitions instancePartitions = new InstancePartitions(
+ instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the partition to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPartitionToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
InstanceConstraintConfig constraintConfig = assignmentConfig.getConstraintConfig();
Review comment:
Did we think about wrapping this under a new concrete implementation of `InstanceConstraintApplier`
The current constraint applier rotates the instances to alleviate hotspots but that happens after the `tagPoolSelector` has already selected the candidate instances and sorted them.
Can handle the requirement in this PR by moving the constraint applier inside the `tagPoolSelector` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839022456
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -51,16 +52,34 @@ public InstanceAssignmentDriver(TableConfig tableConfig) {
_tableConfig = tableConfig;
}
+ /**
+ * Assign instances to InstancePartitions object.
+ * @param instancePartitionsType type of instance partitions
+ * @param instanceConfigs list of instance configs
+ * @param partitionToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
+ */
public InstancePartitions assignInstances(InstancePartitionsType instancePartitionsType,
- List<InstanceConfig> instanceConfigs) {
+ List<InstanceConfig> instanceConfigs, Map<Integer, List<String>> partitionToInstancesMap) {
+ boolean shouldRetainInstanceSequence = !partitionToInstancesMap.isEmpty();
String tableNameWithType = _tableConfig.getTableName();
- LOGGER.info("Starting {} instance assignment for table: {}", instancePartitionsType, tableNameWithType);
+ LOGGER.info("Starting {} instance assignment for table: {}. Should retain instance sequence: {}",
+ instancePartitionsType, tableNameWithType, shouldRetainInstanceSequence);
InstanceAssignmentConfig assignmentConfig =
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(_tableConfig, instancePartitionsType);
InstanceTagPoolSelector tagPoolSelector =
new InstanceTagPoolSelector(assignmentConfig.getTagPoolConfig(), tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = tagPoolSelector.selectInstances(instanceConfigs);
+ Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap =
+ tagPoolSelector.selectInstances(instanceConfigs, partitionToInstancesMap);
+
+ InstancePartitions instancePartitions = new InstancePartitions(
+ instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the partition to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPartitionToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
InstanceConstraintConfig constraintConfig = assignmentConfig.getConstraintConfig();
Review comment:
Did we think about wrapping this under a new concrete implementation of `InstanceConstraintApplier`
The current constraint applier rotates the instances to alleviate hotspots but that happens after the `tagPoolSelector` has already selected the candidate instances and sorted them.
Can we handle the requirement in this PR by moving the constraint applier inside the `tagPoolSelector` ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839893027
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitions.java
##########
@@ -58,25 +60,37 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class InstancePartitions {
private static final char PARTITION_REPLICA_GROUP_SEPARATOR = '_';
+ private static final String PARTITIONS_KEY = "partitions";
+ private static final String INSTANCE_SEPARATOR = "/";
private final String _instancePartitionsName;
- private final Map<String, List<String>> _partitionToInstancesMap;
+ private final Map<String, List<String>> _partitionWithReplicaGroupToInstancesMap;
+ private final Map<Integer, List<String>> _partitionToInstancesMap;
private int _numPartitions;
private int _numReplicaGroups;
public InstancePartitions(String instancePartitionsName) {
_instancePartitionsName = instancePartitionsName;
+ _partitionWithReplicaGroupToInstancesMap = new TreeMap<>();
_partitionToInstancesMap = new TreeMap<>();
}
@JsonCreator
private InstancePartitions(
@JsonProperty(value = "instancePartitionsName", required = true) String instancePartitionsName,
- @JsonProperty(value = "partitionToInstancesMap", required = true)
- Map<String, List<String>> partitionToInstancesMap) {
+ @JsonProperty(value = "partitionWithReplicaGroupToInstancesMap", required = true)
+ Map<String, List<String>> partitionWithReplicaGroupToInstancesMap,
+ @JsonProperty(value = "partitionToInstancesMap")
Review comment:
An example is added to the description of the current PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840945946
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -77,7 +95,31 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
InstancePartitions instancePartitions = new InstancePartitions(
instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the pool to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPoolToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
Review comment:
Good question! Although there are two added configs, they actually are not in the same method. `retainInstanceSequence` is in the caller method of `InstanceAssignmentDriver.assignInstances()` method.
`shouldRetainInstanceSequence` is inside the method of `InstanceAssignmentDriver.assignInstances()`.
These two configs denotes the same thing, i.e. whether to denote whether this feature should be enabled or not. I'll add one comment to explain this map will be persisted in ZNode if dryRun is false.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840973148
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -51,16 +52,34 @@ public InstanceAssignmentDriver(TableConfig tableConfig) {
_tableConfig = tableConfig;
}
+ /**
+ * Assign instances to InstancePartitions object.
+ * @param instancePartitionsType type of instance partitions
+ * @param instanceConfigs list of instance configs
+ * @param partitionToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
+ */
public InstancePartitions assignInstances(InstancePartitionsType instancePartitionsType,
- List<InstanceConfig> instanceConfigs) {
+ List<InstanceConfig> instanceConfigs, Map<Integer, List<String>> partitionToInstancesMap) {
+ boolean shouldRetainInstanceSequence = !partitionToInstancesMap.isEmpty();
String tableNameWithType = _tableConfig.getTableName();
- LOGGER.info("Starting {} instance assignment for table: {}", instancePartitionsType, tableNameWithType);
+ LOGGER.info("Starting {} instance assignment for table: {}. Should retain instance sequence: {}",
+ instancePartitionsType, tableNameWithType, shouldRetainInstanceSequence);
InstanceAssignmentConfig assignmentConfig =
InstanceAssignmentConfigUtils.getInstanceAssignmentConfig(_tableConfig, instancePartitionsType);
InstanceTagPoolSelector tagPoolSelector =
new InstanceTagPoolSelector(assignmentConfig.getTagPoolConfig(), tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = tagPoolSelector.selectInstances(instanceConfigs);
+ Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap =
+ tagPoolSelector.selectInstances(instanceConfigs, partitionToInstancesMap);
+
+ InstancePartitions instancePartitions = new InstancePartitions(
+ instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the partition to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPartitionToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
InstanceConstraintConfig constraintConfig = assignmentConfig.getConstraintConfig();
Review comment:
IMO the code will be much cleaner and maintainable if we create a new strategy -- for example we can create a new `InstanceTagPoolSelector` at the cost of duplicating some code that stays the same.
Driver can probably stay the same.
Thoughts ? cc @jasperjiaguo
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a8dec88) into [master](https://codecov.io/gh/apache/pinot/commit/91c9ce4aebca3913065e08263c6e80f7f515dd50?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (91c9ce4) will **increase** coverage by `6.47%`.
> The diff coverage is `69.75%`.
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
============================================
+ Coverage 64.21% 70.68% +6.47%
- Complexity 4282 4283 +1
============================================
Files 1618 1664 +46
Lines 85376 87433 +2057
Branches 13004 13243 +239
============================================
+ Hits 54820 61801 +6981
+ Misses 26576 21333 -5243
- Partials 3980 4299 +319
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `27.11% <3.08%> (?)` | |
| integration2 | `25.98% <0.61%> (?)` | |
| unittests1 | `67.03% <0.00%> (-0.02%)` | :arrow_down: |
| unittests2 | `14.25% <67.90%> (+0.09%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `23.43% <3.12%> (+23.43%)` | :arrow_up: |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `77.71% <33.33%> (+0.50%)` | :arrow_up: |
| [...ources/PinotInstanceAssignmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90SW5zdGFuY2VBc3NpZ25tZW50UmVzdGxldFJlc291cmNlLmphdmE=) | `70.99% <63.63%> (-2.24%)` | :arrow_down: |
| [...nce/RetainedSequenceInstanceConstraintApplier.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvUmV0YWluZWRTZXF1ZW5jZUluc3RhbmNlQ29uc3RyYWludEFwcGxpZXIuamF2YQ==) | `92.85% <92.85%> (ø)` | |
| [.../assignment/instance/InstanceAssignmentDriver.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VBc3NpZ25tZW50RHJpdmVyLmphdmE=) | `97.82% <95.83%> (+1.82%)` | :arrow_up: |
| [...e/assignment/instance/InstanceTagPoolSelector.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VUYWdQb29sU2VsZWN0b3IuamF2YQ==) | `96.07% <96.72%> (-0.22%)` | :arrow_down: |
| [...oller/api/resources/PinotTableRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90VGFibGVSZXN0bGV0UmVzb3VyY2UuamF2YQ==) | `50.34% <100.00%> (+6.05%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `67.79% <100.00%> (+3.57%)` | :arrow_up: |
| [...ance/HashBasedRotateInstanceConstraintApplier.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSGFzaEJhc2VkUm90YXRlSW5zdGFuY2VDb25zdHJhaW50QXBwbGllci5qYXZh) | `93.33% <100.00%> (+1.02%)` | :arrow_up: |
| [...he/pinot/segment/local/segment/store/IndexKey.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9zZWdtZW50L3N0b3JlL0luZGV4S2V5LmphdmE=) | `65.00% <0.00%> (-10.00%)` | :arrow_down: |
| ... and [384 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [91c9ce4...a8dec88](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840879048
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -77,7 +95,31 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
InstancePartitions instancePartitions = new InstancePartitions(
instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the pool to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPoolToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
Review comment:
Does this mean if we do not set `shouldRetainInstanceSequence` in the initial assignment, the subsequent table rebalance will not be able to persist/retain sequence (since the map will be null) even if we set `retainInstanceSequence` in the rebalance config? Is it okay that we always persist the instance sequence in the initial assignment?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1085383347
> I don't follow why we need to store the pool to instances mapping in the ZK. Essentially what we need is to pass the existing instance partitions to the `InstanceReplicaGroupPartitionSelector.selectInstances()`, and add a flag to enable `minimizeMovement` and use a greedy algorithm to retain as much instances as possible from the candidate instances from each pool.
Yes the initial plan is to reuse the existing instance partitions, but the thing is that for the instance which no longer exists in the current list of instance configs (which is gone), there is no way to know which pool it came from; i.e. even though we know there is an empty seat there, we don't know which pool we should select the new instance from.
As long as we can control the input parameters of `replicaPartitionSelector.selectInstances()` (the input instancePartitions is always empty, thus as long as we control the instance sequence of `poolToInstanceConfigsMap`), we can always get the deterministic instancePartitions as output.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1086320784
> > > I don't follow why we need to store the pool to instances mapping in the ZK. Essentially what we need is to pass the existing instance partitions to the `InstanceReplicaGroupPartitionSelector.selectInstances()`, and add a flag to enable `minimizeMovement` and use a greedy algorithm to retain as much instances as possible from the candidate instances from each pool.
> >
> >
> > Yes the initial plan is to reuse the existing instance partitions, but the thing is that for the instance which no longer exists in the current list of instance configs (which is gone), there is no way to know which pool it came from; i.e. even though we know there is an empty seat there, we don't know which pool we should select the new instance from. As long as we can control the input parameters of `replicaPartitionSelector.selectInstances()` (the input instancePartitions is always empty, thus as long as we control the instance sequence of `poolToInstanceConfigsMap`), we can always get the deterministic instancePartitions as output.
>
> Actually we can know the pool where the original instance is coming from without storing the pool information. The pool selection is deterministic, and is based on the hash code of the table name. Following the same algorithm, for each replica id, you can get the same pool id as the original assignment. Then you may run the greedy algorithm to pick the instances from the candidate instances from the pool to achieve minimum movement.
IMO there is no hard requirement for a pool id to be mapped 1:N to a replica id, right? It's just in current strategy of `InstanceReplicaGroupPartitionSelector` we assign instances from a pool to a replica. But this should not be enforced for future use, especially right now we are implementing selector with FD awareness and it can have instances from multiple pools in 1 replica group.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839050322
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitions.java
##########
@@ -58,25 +60,37 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class InstancePartitions {
private static final char PARTITION_REPLICA_GROUP_SEPARATOR = '_';
+ private static final String PARTITIONS_KEY = "partitions";
+ private static final String INSTANCE_SEPARATOR = "/";
private final String _instancePartitionsName;
- private final Map<String, List<String>> _partitionToInstancesMap;
+ private final Map<String, List<String>> _partitionWithReplicaGroupToInstancesMap;
+ private final Map<Integer, List<String>> _partitionToInstancesMap;
Review comment:
Is the key in `_partitionToInstancesMap` the partitionID ? The code below doesn't seem to imply 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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d31a416) into [master](https://codecov.io/gh/apache/pinot/commit/189fd7a818802b825c020addf0211b27dea4174c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (189fd7a) will **decrease** coverage by `1.58%`.
> The diff coverage is `66.19%`.
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
============================================
- Coverage 70.90% 69.32% -1.59%
- Complexity 4279 4281 +2
============================================
Files 1656 1662 +6
Lines 86766 87318 +552
Branches 13084 13224 +140
============================================
- Hits 61525 60531 -994
- Misses 20996 22551 +1555
+ Partials 4245 4236 -9
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `25.99% <2.11%> (-1.45%)` | :arrow_down: |
| unittests1 | `67.01% <0.00%> (+0.04%)` | :arrow_up: |
| unittests2 | `14.18% <64.08%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `23.43% <7.89%> (-17.74%)` | :arrow_down: |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `75.76% <20.00%> (-3.44%)` | :arrow_down: |
| [...e/assignment/instance/InstanceTagPoolSelector.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VUYWdQb29sU2VsZWN0b3IuamF2YQ==) | `97.91% <98.27%> (+1.62%)` | :arrow_up: |
| [...ources/PinotInstanceAssignmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90SW5zdGFuY2VBc3NpZ25tZW50UmVzdGxldFJlc291cmNlLmphdmE=) | `73.43% <100.00%> (+0.20%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `67.23% <100.00%> (-0.07%)` | :arrow_down: |
| [.../assignment/instance/InstanceAssignmentDriver.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VBc3NpZ25tZW50RHJpdmVyLmphdmE=) | `97.67% <100.00%> (+1.67%)` | :arrow_up: |
| [...g/apache/pinot/server/api/resources/ErrorInfo.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zZXJ2ZXIvYXBpL3Jlc291cmNlcy9FcnJvckluZm8uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...pinot/minion/exception/TaskCancelledException.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtbWluaW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9taW5pb24vZXhjZXB0aW9uL1Rhc2tDYW5jZWxsZWRFeGNlcHRpb24uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...nverttorawindex/ConvertToRawIndexTaskExecutor.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtcGx1Z2lucy9waW5vdC1taW5pb24tdGFza3MvcGlub3QtbWluaW9uLWJ1aWx0aW4tdGFza3Mvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3Bpbm90L3BsdWdpbi9taW5pb24vdGFza3MvY29udmVydHRvcmF3aW5kZXgvQ29udmVydFRvUmF3SW5kZXhUYXNrRXhlY3V0b3IuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...e/pinot/common/minion/MergeRollupTaskMetadata.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWluaW9uL01lcmdlUm9sbHVwVGFza01ldGFkYXRhLmphdmE=) | `0.00% <0.00%> (-94.74%)` | :arrow_down: |
| ... and [202 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [189fd7a...d31a416](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c9591ed) into [master](https://codecov.io/gh/apache/pinot/commit/e2053f6776911dcce5f1ef1e32ed35063ca10bea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e2053f6) will **decrease** coverage by `6.55%`.
> The diff coverage is `64.08%`.
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
============================================
- Coverage 70.73% 64.18% -6.56%
+ Complexity 4282 4280 -2
============================================
Files 1662 1617 -45
Lines 87227 85445 -1782
Branches 13205 13021 -184
============================================
- Hits 61703 54844 -6859
- Misses 21238 26620 +5382
+ Partials 4286 3981 -305
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `67.01% <0.00%> (-0.04%)` | :arrow_down: |
| unittests2 | `14.18% <64.08%> (+0.04%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `0.00% <0.00%> (-41.18%)` | :arrow_down: |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `75.76% <20.00%> (-3.44%)` | :arrow_down: |
| [...e/assignment/instance/InstanceTagPoolSelector.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VUYWdQb29sU2VsZWN0b3IuamF2YQ==) | `97.91% <98.27%> (+1.62%)` | :arrow_up: |
| [...ources/PinotInstanceAssignmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90SW5zdGFuY2VBc3NpZ25tZW50UmVzdGxldFJlc291cmNlLmphdmE=) | `73.43% <100.00%> (+0.20%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `63.88% <100.00%> (-3.43%)` | :arrow_down: |
| [.../assignment/instance/InstanceAssignmentDriver.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VBc3NpZ25tZW50RHJpdmVyLmphdmE=) | `97.67% <100.00%> (+1.67%)` | :arrow_up: |
| [...va/org/apache/pinot/core/routing/RoutingTable.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yb3V0aW5nL1JvdXRpbmdUYWJsZS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pinot/common/config/NettyConfig.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL05ldHR5Q29uZmlnLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...a/org/apache/pinot/common/metrics/MinionMeter.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9NaW5pb25NZXRlci5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...g/apache/pinot/common/metrics/ControllerMeter.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9Db250cm9sbGVyTWV0ZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [372 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [e2053f6...c9591ed](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839028089
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitions.java
##########
@@ -58,25 +60,37 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class InstancePartitions {
private static final char PARTITION_REPLICA_GROUP_SEPARATOR = '_';
+ private static final String PARTITIONS_KEY = "partitions";
+ private static final String INSTANCE_SEPARATOR = "/";
private final String _instancePartitionsName;
- private final Map<String, List<String>> _partitionToInstancesMap;
+ private final Map<String, List<String>> _partitionWithReplicaGroupToInstancesMap;
+ private final Map<Integer, List<String>> _partitionToInstancesMap;
private int _numPartitions;
private int _numReplicaGroups;
public InstancePartitions(String instancePartitionsName) {
_instancePartitionsName = instancePartitionsName;
+ _partitionWithReplicaGroupToInstancesMap = new TreeMap<>();
_partitionToInstancesMap = new TreeMap<>();
}
@JsonCreator
private InstancePartitions(
@JsonProperty(value = "instancePartitionsName", required = true) String instancePartitionsName,
- @JsonProperty(value = "partitionToInstancesMap", required = true)
- Map<String, List<String>> partitionToInstancesMap) {
+ @JsonProperty(value = "partitionWithReplicaGroupToInstancesMap", required = true)
+ Map<String, List<String>> partitionWithReplicaGroupToInstancesMap,
+ @JsonProperty(value = "partitionToInstancesMap")
Review comment:
To get a better understanding of the new information in InstancePartitions, can you update the PR description with an example of what it looks like now ?
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitions.java
##########
@@ -58,25 +60,37 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class InstancePartitions {
private static final char PARTITION_REPLICA_GROUP_SEPARATOR = '_';
+ private static final String PARTITIONS_KEY = "partitions";
+ private static final String INSTANCE_SEPARATOR = "/";
private final String _instancePartitionsName;
- private final Map<String, List<String>> _partitionToInstancesMap;
+ private final Map<String, List<String>> _partitionWithReplicaGroupToInstancesMap;
+ private final Map<Integer, List<String>> _partitionToInstancesMap;
private int _numPartitions;
private int _numReplicaGroups;
public InstancePartitions(String instancePartitionsName) {
_instancePartitionsName = instancePartitionsName;
+ _partitionWithReplicaGroupToInstancesMap = new TreeMap<>();
_partitionToInstancesMap = new TreeMap<>();
}
@JsonCreator
private InstancePartitions(
@JsonProperty(value = "instancePartitionsName", required = true) String instancePartitionsName,
- @JsonProperty(value = "partitionToInstancesMap", required = true)
- Map<String, List<String>> partitionToInstancesMap) {
+ @JsonProperty(value = "partitionWithReplicaGroupToInstancesMap", required = true)
+ Map<String, List<String>> partitionWithReplicaGroupToInstancesMap,
+ @JsonProperty(value = "partitionToInstancesMap")
Review comment:
To get a better understanding of the new information in `InstancePartitions`, can you update the PR description with an example of what it looks like now ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839066546
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +51,108 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param partitionToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> partitionToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // Filter out the instances with the correct tag.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : partitionToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
Review comment:
InstanceConfig already has the InstanceName. Why do we need to store it as key ?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] siddharthteotia commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
siddharthteotia commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r839088155
##########
File path: pinot-common/src/main/java/org/apache/pinot/common/assignment/InstancePartitions.java
##########
@@ -58,25 +60,37 @@
@JsonIgnoreProperties(ignoreUnknown = true)
public class InstancePartitions {
private static final char PARTITION_REPLICA_GROUP_SEPARATOR = '_';
+ private static final String PARTITIONS_KEY = "partitions";
+ private static final String INSTANCE_SEPARATOR = "/";
private final String _instancePartitionsName;
- private final Map<String, List<String>> _partitionToInstancesMap;
+ private final Map<String, List<String>> _partitionWithReplicaGroupToInstancesMap;
+ private final Map<Integer, List<String>> _partitionToInstancesMap;
Review comment:
One other thing to consider is if we can do this by using the existing info in InstancePartitions and not preserving another map that increases ZNode size
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d31a416) into [master](https://codecov.io/gh/apache/pinot/commit/189fd7a818802b825c020addf0211b27dea4174c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (189fd7a) will **decrease** coverage by `0.15%`.
> The diff coverage is `67.60%`.
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
============================================
- Coverage 70.90% 70.74% -0.16%
- Complexity 4279 4281 +2
============================================
Files 1656 1662 +6
Lines 86766 87318 +552
Branches 13084 13224 +140
============================================
+ Hits 61525 61777 +252
- Misses 20996 21267 +271
- Partials 4245 4274 +29
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `27.17% <4.92%> (-1.66%)` | :arrow_down: |
| integration2 | `25.99% <2.11%> (-1.45%)` | :arrow_down: |
| unittests1 | `67.01% <0.00%> (+0.04%)` | :arrow_up: |
| unittests2 | `14.18% <64.08%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `23.43% <7.89%> (-17.74%)` | :arrow_down: |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `77.71% <33.33%> (-1.49%)` | :arrow_down: |
| [...e/assignment/instance/InstanceTagPoolSelector.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VUYWdQb29sU2VsZWN0b3IuamF2YQ==) | `97.91% <98.27%> (+1.62%)` | :arrow_up: |
| [...ources/PinotInstanceAssignmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90SW5zdGFuY2VBc3NpZ25tZW50UmVzdGxldFJlc291cmNlLmphdmE=) | `73.43% <100.00%> (+0.20%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `67.30% <100.00%> (ø)` | |
| [.../assignment/instance/InstanceAssignmentDriver.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VBc3NpZ25tZW50RHJpdmVyLmphdmE=) | `97.67% <100.00%> (+1.67%)` | :arrow_up: |
| [...ctionaryBasedSingleColumnDistinctOnlyExecutor.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9xdWVyeS9kaXN0aW5jdC9kaWN0aW9uYXJ5L0RpY3Rpb25hcnlCYXNlZFNpbmdsZUNvbHVtbkRpc3RpbmN0T25seUV4ZWN1dG9yLmphdmE=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [...src/main/java/org/apache/pinot/client/Request.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1JlcXVlc3QuamF2YQ==) | `0.00% <0.00%> (-60.00%)` | :arrow_down: |
| [...pinot/core/operator/filter/BaseFilterOperator.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9vcGVyYXRvci9maWx0ZXIvQmFzZUZpbHRlck9wZXJhdG9yLmphdmE=) | `71.42% <0.00%> (-28.58%)` | :arrow_down: |
| [...t/core/requesthandler/PinotQueryParserFactory.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9yZXF1ZXN0aGFuZGxlci9QaW5vdFF1ZXJ5UGFyc2VyRmFjdG9yeS5qYXZh) | `37.50% <0.00%> (-25.00%)` | :arrow_down: |
| ... and [115 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [189fd7a...d31a416](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] codecov-commenter commented on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1083528279
# [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#8441](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d31a416) into [master](https://codecov.io/gh/apache/pinot/commit/189fd7a818802b825c020addf0211b27dea4174c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (189fd7a) will **decrease** coverage by `56.71%`.
> The diff coverage is `64.08%`.
```diff
@@ Coverage Diff @@
## master #8441 +/- ##
=============================================
- Coverage 70.90% 14.18% -56.72%
+ Complexity 4279 84 -4195
=============================================
Files 1656 1617 -39
Lines 86766 85437 -1329
Branches 13084 13021 -63
=============================================
- Hits 61525 12123 -49402
- Misses 20996 72399 +51403
+ Partials 4245 915 -3330
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1 | `?` | |
| integration2 | `?` | |
| unittests1 | `?` | |
| unittests2 | `14.18% <64.08%> (-0.03%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/assignment/InstancePartitions.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `0.00% <0.00%> (-41.18%)` | :arrow_down: |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `75.76% <20.00%> (-3.44%)` | :arrow_down: |
| [...e/assignment/instance/InstanceTagPoolSelector.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VUYWdQb29sU2VsZWN0b3IuamF2YQ==) | `97.91% <98.27%> (+1.62%)` | :arrow_up: |
| [...ources/PinotInstanceAssignmentRestletResource.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9hcGkvcmVzb3VyY2VzL1Bpbm90SW5zdGFuY2VBc3NpZ25tZW50UmVzdGxldFJlc291cmNlLmphdmE=) | `73.43% <100.00%> (+0.20%)` | :arrow_up: |
| [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `63.81% <100.00%> (-3.49%)` | :arrow_down: |
| [.../assignment/instance/InstanceAssignmentDriver.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL2Fzc2lnbm1lbnQvaW5zdGFuY2UvSW5zdGFuY2VBc3NpZ25tZW50RHJpdmVyLmphdmE=) | `97.67% <100.00%> (+1.67%)` | :arrow_up: |
| [...ain/java/org/apache/pinot/core/data/table/Key.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL0tleS5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/spi/utils/BooleanUtils.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3Qtc3BpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9zcGkvdXRpbHMvQm9vbGVhblV0aWxzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/data/table/Record.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL3RhYmxlL1JlY29yZC5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../java/org/apache/pinot/core/util/GroupByUtils.java](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS91dGlsL0dyb3VwQnlVdGlscy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [1337 more](https://codecov.io/gh/apache/pinot/pull/8441/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [189fd7a...d31a416](https://codecov.io/gh/apache/pinot/pull/8441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jtao15 commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jtao15 commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840126785
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java
##########
@@ -145,6 +145,8 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
tableConfig.getRoutingConfig().getInstanceSelectorType());
boolean bestEfforts = rebalanceConfig.getBoolean(RebalanceConfigConstants.BEST_EFFORTS,
RebalanceConfigConstants.DEFAULT_BEST_EFFORTS);
+ boolean retainInstanceSequence = rebalanceConfig.getBoolean(RebalanceConfigConstants.RETAIN_INSTANCE_SEQUENCE,
+ RebalanceConfigConstants.DEFAULT_RETAIN_INSTANCE_SEQUENCE);
Review comment:
Should we throw exceptions if `retainInstanceSequence = true` and `reassignInstances = true` or the table does not allow instance assignment?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -121,12 +190,40 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
LOGGER.info("Selecting pools: {} for table: {}", poolsToSelect, _tableNameWithType);
pools.retainAll(poolsToSelect);
} else {
- // Non-pool based selection
+ // Non-pool based selection. All the instances should be associated with a single pool, which is always 0.
+ // E.g.: Pool0 -> [ I1, I2, I3, I4, I5, I6 ]
- LOGGER.info("Selecting {} instances for table: {}", numCandidateInstances, _tableNameWithType);
+ LOGGER.info("Selecting {} instances for table: {}", candidateInstanceConfigsMap.size(), _tableNameWithType);
// Put all instance configs as pool 0
- poolToInstanceConfigsMap.put(0, candidateInstanceConfigs);
+
+ for (Map.Entry<Integer, List<String>> entry : existingPoolToInstancesMap.entrySet()) {
Review comment:
This assume the pools are not changed between `existingInstancePartitions` and `instanceConfigs`. Is it possible that all the instances in the same pool got removed? In other words, is it possible that `poolToNewInstanceConfigsMap.get(pool)` is empty because the pools are changed?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +52,117 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param existingPoolToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> existingPoolToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // If existingPoolToInstancesMap is null, treat it as an empty map.
+ if (existingPoolToInstancesMap == null) {
+ existingPoolToInstancesMap = Collections.emptyMap();
+ }
+ // Filter out the instances with the correct tag.
+ // Use LinkedHashMap here to retain the sorted list of instance names.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ // A deque is used here in order to retain the sequence,
+ // given the fact that the list of instance configs is always sorted.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : existingPoolToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ Map<Integer, List<InstanceConfig>> poolToLatestInstanceConfigsMap = new TreeMap<>();
if (_tagPoolConfig.isPoolBased()) {
- // Pool based selection
+ // Pool based selection. All the instances should be associated with a specific pool number.
+ // Instance selection should be done within the same pool.
+ // E.g.: Pool0 -> [ I1, I2, I3 ]
+ // Pool1 -> [ I4, I5, I6 ]
- // Extract the pool information from the instance configs
- for (InstanceConfig instanceConfig : candidateInstanceConfigs) {
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
+ // Each pool number associates with a list of newly added instance configs,
+ // so that new instances can be fetched from this list.
+ Map<Integer, Deque<InstanceConfig>> poolToNewInstanceConfigsMap = new HashMap<>();
+
+ // Extract the pool information from the instance configs.
+ for (Map.Entry<String, InstanceConfig> entry : candidateInstanceConfigsMap.entrySet()) {
+ String instanceName = entry.getKey();
+ InstanceConfig instanceConfig = entry.getValue();
Map<String, String> poolMap = instanceConfig.getRecord().getMapField(InstanceUtils.POOL_KEY);
if (poolMap != null && poolMap.containsKey(tag)) {
int pool = Integer.parseInt(poolMap.get(tag));
- poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new ArrayList<>()).add(instanceConfig);
+ poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new TreeMap<>()).put(instanceName, instanceConfig);
+ if (newlyAddedInstances.contains(instanceName)) {
+ poolToNewInstanceConfigsMap.computeIfAbsent(pool, k -> new LinkedList<>()).add(instanceConfig);
+ }
+ }
+ }
+
+ for (Map.Entry<Integer, List<String>> entry : existingPoolToInstancesMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<String> existingInstanceAssignmentInPool = entry.getValue();
+ List<InstanceConfig> candidateInstanceConfigsWithSequence = new ArrayList<>();
+ for (String existingInstance: existingInstanceAssignmentInPool) {
+ InstanceConfig instanceConfig = poolToInstanceConfigsMap.get(pool).get(existingInstance);
+ // Add instances to the candidate list and respect the sequence of the existing instances from the ZK.
+ // The missing/removed instances will be replaced by the newly instances.
+ // If the instance still exists from ZK, then add it to the candidate list.
+ // E.g. if the old instances are: [I1, I2, I3, I4] and the new instance are: [I1, I3, I4, I5, I6],
+ // the removed instance is I2 and the newly added instances are I5 and I6.
+ // The position of I2 would be replaced by I5, the new remaining I6 would be appended to the tail.
+ // Thus, the new order would be [I1, I5, I3, I4, I6].
Review comment:
`HashBasedRotateInstanceConstraintApplier` will rotate the list which depends on the number of instances, even if we retain the instances order here, after applying the constraints, the order can be different?
Say we need to select 4 instances (2 replicas * 2 instances/replica), and the rotated list is `[I6, I1, I5, I3, I4]`, then we will pick `[I6, I1, I5, I3]`. This won't guarantee minimum segments movement because we need to move all segments from `I4` to `I5` or `I6`. Is this a valid case?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840818830
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +52,117 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param existingPoolToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> existingPoolToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // If existingPoolToInstancesMap is null, treat it as an empty map.
+ if (existingPoolToInstancesMap == null) {
+ existingPoolToInstancesMap = Collections.emptyMap();
+ }
+ // Filter out the instances with the correct tag.
+ // Use LinkedHashMap here to retain the sorted list of instance names.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ // A deque is used here in order to retain the sequence,
+ // given the fact that the list of instance configs is always sorted.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : existingPoolToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ Map<Integer, List<InstanceConfig>> poolToLatestInstanceConfigsMap = new TreeMap<>();
if (_tagPoolConfig.isPoolBased()) {
- // Pool based selection
+ // Pool based selection. All the instances should be associated with a specific pool number.
+ // Instance selection should be done within the same pool.
+ // E.g.: Pool0 -> [ I1, I2, I3 ]
+ // Pool1 -> [ I4, I5, I6 ]
- // Extract the pool information from the instance configs
- for (InstanceConfig instanceConfig : candidateInstanceConfigs) {
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
+ // Each pool number associates with a list of newly added instance configs,
+ // so that new instances can be fetched from this list.
+ Map<Integer, Deque<InstanceConfig>> poolToNewInstanceConfigsMap = new HashMap<>();
+
+ // Extract the pool information from the instance configs.
+ for (Map.Entry<String, InstanceConfig> entry : candidateInstanceConfigsMap.entrySet()) {
+ String instanceName = entry.getKey();
+ InstanceConfig instanceConfig = entry.getValue();
Map<String, String> poolMap = instanceConfig.getRecord().getMapField(InstanceUtils.POOL_KEY);
if (poolMap != null && poolMap.containsKey(tag)) {
int pool = Integer.parseInt(poolMap.get(tag));
- poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new ArrayList<>()).add(instanceConfig);
+ poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new TreeMap<>()).put(instanceName, instanceConfig);
+ if (newlyAddedInstances.contains(instanceName)) {
+ poolToNewInstanceConfigsMap.computeIfAbsent(pool, k -> new LinkedList<>()).add(instanceConfig);
+ }
+ }
+ }
+
+ for (Map.Entry<Integer, List<String>> entry : existingPoolToInstancesMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<String> existingInstanceAssignmentInPool = entry.getValue();
+ List<InstanceConfig> candidateInstanceConfigsWithSequence = new ArrayList<>();
+ for (String existingInstance: existingInstanceAssignmentInPool) {
+ InstanceConfig instanceConfig = poolToInstanceConfigsMap.get(pool).get(existingInstance);
+ // Add instances to the candidate list and respect the sequence of the existing instances from the ZK.
+ // The missing/removed instances will be replaced by the newly instances.
+ // If the instance still exists from ZK, then add it to the candidate list.
+ // E.g. if the old instances are: [I1, I2, I3, I4] and the new instance are: [I1, I3, I4, I5, I6],
+ // the removed instance is I2 and the newly added instances are I5 and I6.
+ // The position of I2 would be replaced by I5, the new remaining I6 would be appended to the tail.
+ // Thus, the new order would be [I1, I5, I3, I4, I6].
Review comment:
Yes this is a valid case, if the pool exists in the existingPoolToInstancesMap, we actually don't need to rotate the list. Otherwise, just do the rotation as the current behavior. Add a class called `RetainedSequenceInstanceConstraintApplier` to extend the existing default applier.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840879048
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -77,7 +95,31 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
InstancePartitions instancePartitions = new InstancePartitions(
instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the pool to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPoolToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
Review comment:
Does this mean if we do not set `shouldRetainInstanceSequence` in the initial assignment, the subsequent table rebalance will not be able to retain sequence even if we set `retainInstanceSequence` in the rebalance config? Is it okay that we always persist the instance sequence in the initial assignment?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840924957
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentDriver.java
##########
@@ -77,7 +95,31 @@ public InstancePartitions assignInstances(InstancePartitionsType instancePartiti
new InstanceReplicaGroupPartitionSelector(assignmentConfig.getReplicaGroupPartitionConfig(), tableNameWithType);
InstancePartitions instancePartitions = new InstancePartitions(
instancePartitionsType.getInstancePartitionsName(TableNameBuilder.extractRawTableName(tableNameWithType)));
+ if (shouldRetainInstanceSequence) {
+ // Keep the pool to instances map if instance sequence should be retained.
+ instancePartitions
+ .setPoolToInstancesMap(extractInstanceNamesFromPoolToInstanceConfigsMap(poolToInstanceConfigsMap));
+ }
Review comment:
I'm wondering would it be better if we can have one config instead of two to avoid some confusion. I mean as a user I might expect the `retainInstanceSequence` in rebalance to retain current sequence, whereas it's actually a "best effort" depending on if the corresponding flag is set in the initial assignment. Or we can rename the flag in rebalance config to avoid the confusion.
Similarly for current large use-cases without this map already available we might need some migration plan?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo edited a comment on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo edited a comment on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1086320784
> > > I don't follow why we need to store the pool to instances mapping in the ZK. Essentially what we need is to pass the existing instance partitions to the `InstanceReplicaGroupPartitionSelector.selectInstances()`, and add a flag to enable `minimizeMovement` and use a greedy algorithm to retain as much instances as possible from the candidate instances from each pool.
> >
> >
> > Yes the initial plan is to reuse the existing instance partitions, but the thing is that for the instance which no longer exists in the current list of instance configs (which is gone), there is no way to know which pool it came from; i.e. even though we know there is an empty seat there, we don't know which pool we should select the new instance from. As long as we can control the input parameters of `replicaPartitionSelector.selectInstances()` (the input instancePartitions is always empty, thus as long as we control the instance sequence of `poolToInstanceConfigsMap`), we can always get the deterministic instancePartitions as output.
>
> Actually we can know the pool where the original instance is coming from without storing the pool information. The pool selection is deterministic, and is based on the hash code of the table name. Following the same algorithm, for each replica id, you can get the same pool id as the original assignment. Then you may run the greedy algorithm to pick the instances from the candidate instances from the pool to achieve minimum movement.
IMO there is no hard requirement for a pool id to be mapped ~~1:1~~ 1:N to a replica id, right? It's just in current strategy of `InstanceReplicaGroupPartitionSelector` we assign instances to a replica from one pool. But this should not be enforced for future use, especially right now we are implementing selector with FD awareness and it can have instances from multiple pools in 1 replica group.
In other words we should not rely on the status-quo of we can "reverse engineering the pool id from replica group id". So I think the pool -> instance mapping should probably be saved.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jackjlli commented on pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jackjlli commented on pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#issuecomment-1086373452
> If we want to achieve minimum segments movement, we should guarantee the pool number is not changed for a replica group before and after the rebalance, else we are moving one replica from a set of hosts to another. The pool selection will be deterministic if the number of pools remains the same. Maybe we can set the scope to only handle the case that the number of pools didn't change.
Actually this is hard in real case. Supposed if there is one whole pool that needs to be decommissioned and the other pools remain the same, your assumption will break. If we give up / disable this feature in the current table rebalance in order to accept a new pool, the instance assignment in other pools will be affected.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [pinot] jasperjiaguo commented on a change in pull request #8441: Add retainInstancesSequence feature to table rebalance to minimize data movement between instances
Posted by GitBox <gi...@apache.org>.
jasperjiaguo commented on a change in pull request #8441:
URL: https://github.com/apache/pinot/pull/8441#discussion_r840982343
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceTagPoolSelector.java
##########
@@ -48,52 +52,117 @@ public InstanceTagPoolSelector(InstanceTagPoolConfig tagPoolConfig, String table
/**
* Returns a map from pool to instance configs based on the tag and pool config for the given instance configs.
+ * @param instanceConfigs list of latest instance configs from ZK.
+ * @param existingPoolToInstancesMap existing instance with sequence that should be respected. An empty list
+ * means no preceding sequence to respect and the instances would be sorted.
*/
- public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs) {
+ public Map<Integer, List<InstanceConfig>> selectInstances(List<InstanceConfig> instanceConfigs,
+ Map<Integer, List<String>> existingPoolToInstancesMap) {
int tableNameHash = Math.abs(_tableNameWithType.hashCode());
LOGGER.info("Starting instance tag/pool selection for table: {} with hash: {}", _tableNameWithType, tableNameHash);
- // Filter out the instances with the correct tag
+ // If existingPoolToInstancesMap is null, treat it as an empty map.
+ if (existingPoolToInstancesMap == null) {
+ existingPoolToInstancesMap = Collections.emptyMap();
+ }
+ // Filter out the instances with the correct tag.
+ // Use LinkedHashMap here to retain the sorted list of instance names.
String tag = _tagPoolConfig.getTag();
- List<InstanceConfig> candidateInstanceConfigs = new ArrayList<>();
+ Map<String, InstanceConfig> candidateInstanceConfigsMap = new LinkedHashMap<>();
for (InstanceConfig instanceConfig : instanceConfigs) {
if (instanceConfig.getTags().contains(tag)) {
- candidateInstanceConfigs.add(instanceConfig);
+ candidateInstanceConfigsMap.put(instanceConfig.getInstanceName(), instanceConfig);
}
}
- candidateInstanceConfigs.sort(Comparator.comparing(InstanceConfig::getInstanceName));
- int numCandidateInstances = candidateInstanceConfigs.size();
+
+ // Find out newly added instances from the latest copies of instance configs.
+ // A deque is used here in order to retain the sequence,
+ // given the fact that the list of instance configs is always sorted.
+ Deque<String> newlyAddedInstances = new LinkedList<>(candidateInstanceConfigsMap.keySet());
+ for (List<String> existingInstancesWithSequence : existingPoolToInstancesMap.values()) {
+ newlyAddedInstances.removeAll(existingInstancesWithSequence);
+ }
+
+ int numCandidateInstances = candidateInstanceConfigsMap.size();
Preconditions.checkState(numCandidateInstances > 0, "No enabled instance has the tag: %s", tag);
LOGGER.info("{} enabled instances have the tag: {} for table: {}", numCandidateInstances, tag, _tableNameWithType);
- Map<Integer, List<InstanceConfig>> poolToInstanceConfigsMap = new TreeMap<>();
+ Map<Integer, List<InstanceConfig>> poolToLatestInstanceConfigsMap = new TreeMap<>();
if (_tagPoolConfig.isPoolBased()) {
- // Pool based selection
+ // Pool based selection. All the instances should be associated with a specific pool number.
+ // Instance selection should be done within the same pool.
+ // E.g.: Pool0 -> [ I1, I2, I3 ]
+ // Pool1 -> [ I4, I5, I6 ]
- // Extract the pool information from the instance configs
- for (InstanceConfig instanceConfig : candidateInstanceConfigs) {
+ // Each pool number associates with a map that key is the instance name and value is the instance config.
+ Map<Integer, Map<String, InstanceConfig>> poolToInstanceConfigsMap = new HashMap<>();
+ // Each pool number associates with a list of newly added instance configs,
+ // so that new instances can be fetched from this list.
+ Map<Integer, Deque<InstanceConfig>> poolToNewInstanceConfigsMap = new HashMap<>();
+
+ // Extract the pool information from the instance configs.
+ for (Map.Entry<String, InstanceConfig> entry : candidateInstanceConfigsMap.entrySet()) {
+ String instanceName = entry.getKey();
+ InstanceConfig instanceConfig = entry.getValue();
Map<String, String> poolMap = instanceConfig.getRecord().getMapField(InstanceUtils.POOL_KEY);
if (poolMap != null && poolMap.containsKey(tag)) {
int pool = Integer.parseInt(poolMap.get(tag));
- poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new ArrayList<>()).add(instanceConfig);
+ poolToInstanceConfigsMap.computeIfAbsent(pool, k -> new TreeMap<>()).put(instanceName, instanceConfig);
+ if (newlyAddedInstances.contains(instanceName)) {
+ poolToNewInstanceConfigsMap.computeIfAbsent(pool, k -> new LinkedList<>()).add(instanceConfig);
+ }
+ }
+ }
+
+ for (Map.Entry<Integer, List<String>> entry : existingPoolToInstancesMap.entrySet()) {
+ Integer pool = entry.getKey();
+ List<String> existingInstanceAssignmentInPool = entry.getValue();
+ List<InstanceConfig> candidateInstanceConfigsWithSequence = new ArrayList<>();
+ for (String existingInstance: existingInstanceAssignmentInPool) {
+ InstanceConfig instanceConfig = poolToInstanceConfigsMap.get(pool).get(existingInstance);
+ // Add instances to the candidate list and respect the sequence of the existing instances from the ZK.
+ // The missing/removed instances will be replaced by the newly instances.
+ // If the instance still exists from ZK, then add it to the candidate list.
+ // E.g. if the old instances are: [I1, I2, I3, I4] and the new instance are: [I1, I3, I4, I5, I6],
+ // the removed instance is I2 and the newly added instances are I5 and I6.
+ // The position of I2 would be replaced by I5, the new remaining I6 would be appended to the tail.
+ // Thus, the new order would be [I1, I5, I3, I4, I6].
Review comment:
+1 on a follow up PR. I think for the uplift problem we should solve it in/extending InstanceReplicaGroupPartitionSelector.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org