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-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
