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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
