You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "summerhasama-stripe (via GitHub)" <gi...@apache.org> on 2023/07/10 17:27:43 UTC
[GitHub] [pinot] summerhasama-stripe opened a new pull request, #11073: For table rebalance, check if instances are equal for NO_OP
summerhasama-stripe opened a new pull request, #11073:
URL: https://github.com/apache/pinot/pull/11073
For table rebalance, if reassigning instances we want to check if both the instance and segment assignment is unchanged to determine NO_OP.
Tested: deployed the change and tried rebalance for a table that didn't need changing and it returned NO_OP. Tested again with a changed tenant and ran rebalance and it returned DONE where the instanceAssignnmnetUnchanged log was 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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1262628811
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -256,22 +264,15 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
tierToInstancePartitionsMap, null);
}
- if (currentAssignment.equals(targetAssignment)) {
+ boolean segmentAssignmentUnchanged = currentAssignment.equals(targetAssignment);
+ LOGGER.info("For rebalanceId: {}, segmentAssignmentUnchanged: {}, "
+ + "tierInstancePartitionsUnchanged: {}, instancePartitionsUnchanged: {}",
+ rebalanceJobId, segmentAssignmentUnchanged, tierInstancePartitionsUnchanged, instancePartitionsUnchanged);
+
+ if (segmentAssignmentUnchanged && tierInstancePartitionsUnchanged && instancePartitionsUnchanged) {
Review Comment:
Why are we always returning IN_PROGRESS? https://github.com/apache/pinot/blob/master/pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java#L683-L691
--
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 merged pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #11073:
URL: https://github.com/apache/pinot/pull/11073
--
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 a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1261580052
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -226,8 +226,12 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
// Calculate instance partitions map
Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap;
+ Boolean instancePartitionsUnchanged;
Review Comment:
(minor)
```suggestion
boolean instancePartitionsUnchanged;
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -480,22 +483,32 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
}
}
- private Map<InstancePartitionsType, InstancePartitions> getInstancePartitionsMap(TableConfig tableConfig,
- boolean reassignInstances, boolean bootstrap, boolean dryRun) {
+ /**
+ * Gets the instance partitions for instance partition types and also returns a boolean for whether they are unchanged
+ */
+ private Pair<Map<InstancePartitionsType, InstancePartitions>, Boolean> getInstancePartitionsMap(
Review Comment:
(Optional) Might be more readable if we pass in a `MutableBoolean`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -531,7 +547,7 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
InstancePartitionsUtils.persistInstancePartitions(_helixManager.getHelixPropertyStore(),
instancePartitions);
}
- return instancePartitions;
+ return Pair.of(instancePartitions, true);
Review Comment:
We want to check if it is the same as existing instance partitions. If so, we can also skip the persisting step
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -237,9 +241,13 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
// Calculate instance partitions for tiers if configured
List<Tier> sortedTiers = getSortedTiers(tableConfig);
- Map<String, InstancePartitions> tierToInstancePartitionsMap =
+
+ Pair<Map<String, InstancePartitions>, Boolean> tierToInstancePartitionsMapAndUnchanged =
getTierToInstancePartitionsMap(tableConfig, sortedTiers, reassignInstances, bootstrap, dryRun);
+ Map<String, InstancePartitions> tierToInstancePartitionsMap = tierToInstancePartitionsMapAndUnchanged.getLeft();
+ Boolean tierInstancePartitionsUnchanged = tierToInstancePartitionsMapAndUnchanged.getRight();
Review Comment:
(minor)
```suggestion
boolean tierInstancePartitionsUnchanged = tierToInstancePartitionsMapAndUnchanged.getRight();
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -548,12 +564,12 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
LOGGER.info("Persisting instance partitions: {} to ZK", instancePartitions);
InstancePartitionsUtils.persistInstancePartitions(_helixManager.getHelixPropertyStore(), instancePartitions);
}
- return instancePartitions;
+ return Pair.of(instancePartitions, instancePartitions.equals(existingInstancePartitions));
Review Comment:
We can skip the persisting step when they match
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -256,22 +264,15 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
tierToInstancePartitionsMap, null);
}
- if (currentAssignment.equals(targetAssignment)) {
+ boolean segmentAssignmentUnchanged = currentAssignment.equals(targetAssignment);
+ LOGGER.info("For rebalanceId: {}, segmentAssignmentUnchanged: {}, "
+ + "tierInstancePartitionsUnchanged: {}, instancePartitionsUnchanged: {}",
+ rebalanceJobId, segmentAssignmentUnchanged, tierInstancePartitionsUnchanged, instancePartitionsUnchanged);
+
+ if (segmentAssignmentUnchanged && tierInstancePartitionsUnchanged && instancePartitionsUnchanged) {
Review Comment:
We want to follow the existing logic, and always terminate when segment assignment is not changed:
```
if (segmentAssignmentUnchanged) {
if (instancePartitionsUnchanged && tierInstancePartitionsUnchanged) {
// NO_OP
...
} else {
// DONE
if (dryRun) {
...
} else {
...
}
}
}
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -569,7 +585,7 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
LOGGER.info("Removing instance partitions: {} from ZK if it exists", instancePartitionsName);
InstancePartitionsUtils.removeInstancePartitions(_helixManager.getHelixPropertyStore(), instancePartitionsName);
}
- return instancePartitions;
+ return Pair.of(instancePartitions, true);
Review Comment:
Here we want to check if there is existing instance partitions, and return false if there is no existing instance partitions
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -256,22 +264,15 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
tierToInstancePartitionsMap, null);
}
- if (currentAssignment.equals(targetAssignment)) {
+ boolean segmentAssignmentUnchanged = currentAssignment.equals(targetAssignment);
+ LOGGER.info("For rebalanceId: {}, segmentAssignmentUnchanged: {}, "
Review Comment:
Let's include the table name, and follow the sequence of `instancePartitionsUnchanged`, `tierInstancePartitionsUnchanged`, `segmentAssignmentUnchanged`
--
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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1268165609
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -569,7 +585,7 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
LOGGER.info("Removing instance partitions: {} from ZK if it exists", instancePartitionsName);
InstancePartitionsUtils.removeInstancePartitions(_helixManager.getHelixPropertyStore(), instancePartitionsName);
}
- return instancePartitions;
+ return Pair.of(instancePartitions, true);
Review Comment:
So just to double check, if there are existing instance partitions then it should return true that the instancePartitionsUnchanged?
--
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 a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1271238513
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java:
##########
@@ -246,19 +246,19 @@ public void testRebalance()
tableConfig.setInstanceAssignmentConfigMap(null);
_helixResourceManager.updateTableConfig(tableConfig);
- // Without instances reassignment, the rebalance should return status NO_OP as instance partitions are already
- // generated
+ // Without instances reassignment, the rebalance should return status DONE,
Review Comment:
This doesn't seem correct. Without instance reassignment, we should not remove the instance partitions, and it should return NO_OP. Instance partitions should be removed when instance reassignment is enabled
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -522,38 +550,44 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig, instancePartitionsType);
if (hasPreConfiguredInstancePartitions) {
String referenceInstancePartitionsName = tableConfig.getInstancePartitionsMap().get(instancePartitionsType);
+ InstancePartitions existingInstancePartitions =
Review Comment:
(minor) `existingInstancePartitions` is required in all branches, suggest reading it before getting into the if branch. Same for tier
--
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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1272315952
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java:
##########
@@ -246,19 +246,19 @@ public void testRebalance()
tableConfig.setInstanceAssignmentConfigMap(null);
_helixResourceManager.updateTableConfig(tableConfig);
- // Without instances reassignment, the rebalance should return status NO_OP as instance partitions are already
- // generated
+ // Without instances reassignment, the rebalance should return status DONE,
Review Comment:
https://github.com/apache/pinot/blob/master/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java#L559-L572
This code here will remove instance partitions whether instance reassignment 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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1268165609
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -569,7 +585,7 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
LOGGER.info("Removing instance partitions: {} from ZK if it exists", instancePartitionsName);
InstancePartitionsUtils.removeInstancePartitions(_helixManager.getHelixPropertyStore(), instancePartitionsName);
}
- return instancePartitions;
+ return Pair.of(instancePartitions, true);
Review Comment:
So just to double check, if there are existing instance partitions then it should return NO_OP?
--
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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1272344088
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -522,38 +550,44 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig, instancePartitionsType);
if (hasPreConfiguredInstancePartitions) {
String referenceInstancePartitionsName = tableConfig.getInstancePartitionsMap().get(instancePartitionsType);
+ InstancePartitions existingInstancePartitions =
Review Comment:
Good idea.
--
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] priyen commented on pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "priyen (via GitHub)" <gi...@apache.org>.
priyen commented on PR #11073:
URL: https://github.com/apache/pinot/pull/11073#issuecomment-1629420261
cc @Jackie-Jiang
--
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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1262621911
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -256,22 +264,15 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
tierToInstancePartitionsMap, null);
}
- if (currentAssignment.equals(targetAssignment)) {
+ boolean segmentAssignmentUnchanged = currentAssignment.equals(targetAssignment);
+ LOGGER.info("For rebalanceId: {}, segmentAssignmentUnchanged: {}, "
+ + "tierInstancePartitionsUnchanged: {}, instancePartitionsUnchanged: {}",
+ rebalanceJobId, segmentAssignmentUnchanged, tierInstancePartitionsUnchanged, instancePartitionsUnchanged);
+
+ if (segmentAssignmentUnchanged && tierInstancePartitionsUnchanged && instancePartitionsUnchanged) {
Review Comment:
why would these be DONE if they need changing? Why not IN_PROGRESS
```
// DONE
if (dryRun) {
...
} else {
...
}
```
--
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 a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1271222058
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -480,22 +483,32 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
}
}
- private Map<InstancePartitionsType, InstancePartitions> getInstancePartitionsMap(TableConfig tableConfig,
- boolean reassignInstances, boolean bootstrap, boolean dryRun) {
+ /**
+ * Gets the instance partitions for instance partition types and also returns a boolean for whether they are unchanged
+ */
+ private Pair<Map<InstancePartitionsType, InstancePartitions>, Boolean> getInstancePartitionsMap(
Review Comment:
Currently the method name doesn't match the return value type, but I'm also fine with it
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -480,22 +483,32 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
}
}
- private Map<InstancePartitionsType, InstancePartitions> getInstancePartitionsMap(TableConfig tableConfig,
- boolean reassignInstances, boolean bootstrap, boolean dryRun) {
+ /**
+ * Gets the instance partitions for instance partition types and also returns a boolean for whether they are unchanged
+ */
+ private Pair<Map<InstancePartitionsType, InstancePartitions>, Boolean> getInstancePartitionsMap(
Review Comment:
Currently the method name doesn't match the return value type, but I'm also fine with it (marked as optional)
--
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 #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on PR #11073:
URL: https://github.com/apache/pinot/pull/11073#issuecomment-1633044428
Please take a look at the failed test, and consider adding some new test to validate the 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] codecov-commenter commented on pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #11073:
URL: https://github.com/apache/pinot/pull/11073#issuecomment-1629482327
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/11073?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#11073](https://app.codecov.io/gh/apache/pinot/pull/11073?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (ca3180d) into [master](https://app.codecov.io/gh/apache/pinot/commit/e3cac6428deb891ab66ae9dc5704442b49dc4c0e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (e3cac64) will **decrease** coverage by `0.01%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #11073 +/- ##
==========================================
- Coverage 0.11% 0.11% -0.01%
==========================================
Files 2200 2200
Lines 118818 118872 +54
Branches 17993 18007 +14
==========================================
Hits 137 137
- Misses 118661 118715 +54
Partials 20 20
```
| Flag | Coverage Δ | |
|---|---|---|
| integration1temurin11 | `0.00% <0.00%> (ø)` | |
| integration1temurin17 | `0.00% <0.00%> (ø)` | |
| integration1temurin20 | `?` | |
| integration2temurin11 | `?` | |
| integration2temurin17 | `?` | |
| integration2temurin20 | `?` | |
| unittests1temurin11 | `?` | |
| unittests1temurin17 | `?` | |
| unittests1temurin20 | `?` | |
| unittests2temurin11 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin17 | `0.11% <0.00%> (-0.01%)` | :arrow_down: |
| unittests2temurin20 | `0.11% <0.00%> (-0.01%)` | :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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pinot/pull/11073?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...he/pinot/common/assignment/InstancePartitions.java](https://app.codecov.io/gh/apache/pinot/pull/11073?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnMuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...ntroller/helix/core/rebalance/TableRebalancer.java](https://app.codecov.io/gh/apache/pinot/pull/11073?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYmFsYW5jZS9UYWJsZVJlYmFsYW5jZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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] summerhasama-stripe commented on pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on PR #11073:
URL: https://github.com/apache/pinot/pull/11073#issuecomment-1638828201
@Jackie-Jiang Made the changes you suggested. I can't tell what is causing the tests to fail though.
--
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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1272315952
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java:
##########
@@ -246,19 +246,19 @@ public void testRebalance()
tableConfig.setInstanceAssignmentConfigMap(null);
_helixResourceManager.updateTableConfig(tableConfig);
- // Without instances reassignment, the rebalance should return status NO_OP as instance partitions are already
- // generated
+ // Without instances reassignment, the rebalance should return status DONE,
Review Comment:
https://github.com/apache/pinot/blob/master/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java#L559-L572
This code here will remove instance partitions whether instance reassignment or not. Should I change the if condition?
--
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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1271002967
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -480,22 +483,32 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
}
}
- private Map<InstancePartitionsType, InstancePartitions> getInstancePartitionsMap(TableConfig tableConfig,
- boolean reassignInstances, boolean bootstrap, boolean dryRun) {
+ /**
+ * Gets the instance partitions for instance partition types and also returns a boolean for whether they are unchanged
+ */
+ private Pair<Map<InstancePartitionsType, InstancePartitions>, Boolean> getInstancePartitionsMap(
Review Comment:
How is this more readable?
--
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 a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1272896552
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java:
##########
@@ -246,19 +246,19 @@ public void testRebalance()
tableConfig.setInstanceAssignmentConfigMap(null);
_helixResourceManager.updateTableConfig(tableConfig);
- // Without instances reassignment, the rebalance should return status NO_OP as instance partitions are already
- // generated
+ // Without instances reassignment, the rebalance should return status DONE,
Review Comment:
Yes, you are right. Ideally we should only modify/remove instance partitions when instance reassignment is enabled, but currently that is not the case.
We can fix this in a separate PR because this is not introduced in this PR, and it also affects other classes
--
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 a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1273164674
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancerClusterStatelessTest.java:
##########
@@ -246,19 +246,19 @@ public void testRebalance()
tableConfig.setInstanceAssignmentConfigMap(null);
_helixResourceManager.updateTableConfig(tableConfig);
- // Without instances reassignment, the rebalance should return status NO_OP as instance partitions are already
- // generated
+ // Without instances reassignment, the rebalance should return status DONE,
Review Comment:
Filed #11169 to fix this
--
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 a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1260421100
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -226,7 +226,12 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
// Calculate instance partitions map
Map<InstancePartitionsType, InstancePartitions> instancePartitionsMap;
+ Map<InstancePartitionsType, InstancePartitions> existingInstancePartitionsMap = null;
try {
+ if (reassignInstances) {
+ existingInstancePartitionsMap =
Review Comment:
(Code format) Please reformat the changes with [Pinot Style](https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#setup-ide)
--
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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1268165609
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -569,7 +585,7 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
LOGGER.info("Removing instance partitions: {} from ZK if it exists", instancePartitionsName);
InstancePartitionsUtils.removeInstancePartitions(_helixManager.getHelixPropertyStore(), instancePartitionsName);
}
- return instancePartitions;
+ return Pair.of(instancePartitions, true);
Review Comment:
So just to double check, if there are existing instance partitions then it should return true that the instancePartitionsUnchanged
--
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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1269593406
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -569,7 +585,7 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
LOGGER.info("Removing instance partitions: {} from ZK if it exists", instancePartitionsName);
InstancePartitionsUtils.removeInstancePartitions(_helixManager.getHelixPropertyStore(), instancePartitionsName);
}
- return instancePartitions;
+ return Pair.of(instancePartitions, true);
Review Comment:
I think if there are existing instance partitions, then we remove then from ZK so shouldn't it be DONE (if some exist then 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] Jackie-Jiang commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1270928029
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -522,16 +550,19 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig, instancePartitionsType);
if (hasPreConfiguredInstancePartitions) {
String referenceInstancePartitionsName = tableConfig.getInstancePartitionsMap().get(instancePartitionsType);
+ InstancePartitions existingInstancePartitions = InstancePartitionsUtils.fetchInstancePartitions(
Review Comment:
Here we need to read the table's instance partitions name instead of the reference instance partitions name
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -522,16 +550,19 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
TableConfigUtils.hasPreConfiguredInstancePartitions(tableConfig, instancePartitionsType);
if (hasPreConfiguredInstancePartitions) {
String referenceInstancePartitionsName = tableConfig.getInstancePartitionsMap().get(instancePartitionsType);
+ InstancePartitions existingInstancePartitions = InstancePartitionsUtils.fetchInstancePartitions(
+ _helixManager.getHelixPropertyStore(), referenceInstancePartitionsName);
InstancePartitions instancePartitions =
InstancePartitionsUtils.fetchInstancePartitionsWithRename(_helixManager.getHelixPropertyStore(),
referenceInstancePartitionsName, instancePartitionsType.getInstancePartitionsName(rawTableName));
- if (!dryRun) {
+ boolean instancePartitionsUnchanged = instancePartitions.equals(existingInstancePartitions);
+ if (!dryRun && instancePartitionsUnchanged) {
Review Comment:
If instance partitions is unchanged, we should not persist it
```suggestion
if (!dryRun && !instancePartitionsUnchanged) {
```
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -544,16 +575,17 @@ private InstancePartitions getInstancePartitions(TableConfig tableConfig,
InstancePartitions instancePartitions = instanceAssignmentDriver.assignInstances(instancePartitionsType,
_helixDataAccessor.getChildValues(_helixDataAccessor.keyBuilder().instanceConfigs(), true),
existingInstancePartitions);
- if (!dryRun) {
+ boolean instancePartitionsUnchanged = instancePartitions.equals(existingInstancePartitions);
+ if (!dryRun && instancePartitionsUnchanged) {
Review Comment:
Same here
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -643,17 +688,17 @@ private InstancePartitions getInstancePartitionsForTier(TableConfig tableConfig,
LOGGER.info("Persisting instance partitions: {} to ZK", instancePartitions);
InstancePartitionsUtils.persistInstancePartitions(_helixManager.getHelixPropertyStore(), instancePartitions);
}
- return instancePartitions;
+ return Pair.of(instancePartitions, instancePartitions.equals(existingInstancePartitions));
Review Comment:
We want to follow the same logic as non-tier instance assignment. Also, in `bootstrap` mode, we still want to read the `existingInstancePartitions`, but just passing `null` into `instanceAssignmentDriver.assignInstances()`
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -623,7 +668,7 @@ private InstancePartitions getInstancePartitionsForTier(TableConfig tableConfig,
LOGGER.info("Removing instance partitions: {} from ZK if it exists", instancePartitionsName);
InstancePartitionsUtils.removeInstancePartitions(_helixManager.getHelixPropertyStore(), instancePartitionsName);
}
- return defaultInstancePartitions;
+ return Pair.of(defaultInstancePartitions, true);
Review Comment:
Here we should check if existing instance partitions exist
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -256,22 +264,15 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
tierToInstancePartitionsMap, null);
}
- if (currentAssignment.equals(targetAssignment)) {
+ boolean segmentAssignmentUnchanged = currentAssignment.equals(targetAssignment);
+ LOGGER.info("For rebalanceId: {}, segmentAssignmentUnchanged: {}, "
Review Comment:
(minor) The above comment is not addressed. Also there are double spaces
--
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] summerhasama-stripe commented on a diff in pull request #11073: For table rebalance, check if instances are equal for NO_OP
Posted by "summerhasama-stripe (via GitHub)" <gi...@apache.org>.
summerhasama-stripe commented on code in PR #11073:
URL: https://github.com/apache/pinot/pull/11073#discussion_r1270988818
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/rebalance/TableRebalancer.java:
##########
@@ -256,22 +264,15 @@ public RebalanceResult rebalance(TableConfig tableConfig, Configuration rebalanc
tierToInstancePartitionsMap, null);
}
- if (currentAssignment.equals(targetAssignment)) {
+ boolean segmentAssignmentUnchanged = currentAssignment.equals(targetAssignment);
+ LOGGER.info("For rebalanceId: {}, segmentAssignmentUnchanged: {}, "
Review Comment:
I did add the table name (look below). But I'll fix the double spaces
--
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