Copilot commented on code in PR #19266:
URL: https://github.com/apache/pinot/pull/19266#discussion_r3807470178
##########
pinot-controller/src/test/java/org/apache/pinot/controller/helix/core/assignment/instance/InstanceAssignmentTest.java:
##########
@@ -3393,6 +3393,107 @@ public void
testPoolBasedFDAwareSteadyStateMinimizeDataMovement() {
assertEquals(steadyStatePartitions.getInstances(0, rg),
initialPartitions.getInstances(0, rg));
}
}
+
+ /// FD_AWARE must accept Helix pool tags that do not start at 0 (e.g. 1/2/3).
+ /// Every other pool-based test uses {@code pool = i % numPools}, so this
crash had no coverage.
+ /// See <a href="https://github.com/apache/pinot/issues/12239";>#12239</a>.
+ @Test
+ public void testPoolBasedFDAwareNonZeroBasedPools() {
+ // Same first topology as testPoolBasedFDAware, but pool = (i % numPools)
+ 1 → {1,2,3,4,5}.
+ int numInstances = 21;
+ int numPools = 5;
+ int numReplicaGroups = 3;
+ int numInstancesPerReplicaGroup = numInstances / numReplicaGroups;
+ List<InstanceConfig> instanceConfigs =
+ newFDAwarePoolInstanceConfigs(numInstances, numPools);
+ InstanceTagPoolConfig tagPoolConfig = new
InstanceTagPoolConfig(OFFLINE_TAG, true, numPools, null);
+ InstanceReplicaGroupPartitionConfig replicaPartitionConfig =
+ new InstanceReplicaGroupPartitionConfig(true, 0, numReplicaGroups,
numInstancesPerReplicaGroup, 0, 0, false,
+ null);
+ TableConfig tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(RAW_TABLE_NAME)
+
.setInstanceAssignmentConfigMap(Map.of(InstancePartitionsType.OFFLINE.toString(),
+ new InstanceAssignmentConfig(tagPoolConfig, null,
replicaPartitionConfig,
+
InstanceAssignmentConfig.PartitionSelector.FD_AWARE_INSTANCE_PARTITION_SELECTOR.toString(),
false)))
+ .build();
+ InstanceAssignmentDriver driver = new
InstanceAssignmentDriver(tableConfig);
+ InstancePartitions instancePartitions =
+ driver.assignInstances(InstancePartitionsType.OFFLINE,
instanceConfigs, null);
+ assertFilledUniqueAssignment(instancePartitions, instanceConfigs,
numReplicaGroups,
+ numInstancesPerReplicaGroup);
+
+ // Incremental uplift with minimizeDataMovement hits setExistingInstance
with the raw pool ids.
+ numInstances = 28;
+ numReplicaGroups = 4;
+ numInstancesPerReplicaGroup = numInstances / numReplicaGroups;
+ instanceConfigs = newFDAwarePoolInstanceConfigs(numInstances, numPools);
+ replicaPartitionConfig =
+ new InstanceReplicaGroupPartitionConfig(true, 0, numReplicaGroups,
numInstancesPerReplicaGroup, 0, 0, true,
+ null);
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(RAW_TABLE_NAME)
+
.setInstanceAssignmentConfigMap(Map.of(InstancePartitionsType.OFFLINE.toString(),
+ new InstanceAssignmentConfig(tagPoolConfig, null,
replicaPartitionConfig,
+
InstanceAssignmentConfig.PartitionSelector.FD_AWARE_INSTANCE_PARTITION_SELECTOR.toString(),
true)))
+ .build();
+ driver = new InstanceAssignmentDriver(tableConfig);
+ instancePartitions =
driver.assignInstances(InstancePartitionsType.OFFLINE, instanceConfigs,
instancePartitions);
+ assertFilledUniqueAssignment(instancePartitions, instanceConfigs,
numReplicaGroups,
+ numInstancesPerReplicaGroup);
+
+ // Reporter case: pools {1,2,3}.
+ numInstances = 6;
+ numPools = 3;
+ numReplicaGroups = 3;
+ numInstancesPerReplicaGroup = 2;
+ instanceConfigs = newFDAwarePoolInstanceConfigs(numInstances, numPools);
+ tagPoolConfig = new InstanceTagPoolConfig(OFFLINE_TAG, true, numPools,
null);
+ replicaPartitionConfig =
+ new InstanceReplicaGroupPartitionConfig(true, 0, numReplicaGroups,
numInstancesPerReplicaGroup, 0, 0, false,
+ null);
+ tableConfig = new
TableConfigBuilder(TableType.OFFLINE).setTableName(RAW_TABLE_NAME)
+
.setInstanceAssignmentConfigMap(Map.of(InstancePartitionsType.OFFLINE.toString(),
+ new InstanceAssignmentConfig(tagPoolConfig, null,
replicaPartitionConfig,
+
InstanceAssignmentConfig.PartitionSelector.FD_AWARE_INSTANCE_PARTITION_SELECTOR.toString(),
false)))
+ .build();
+ driver = new InstanceAssignmentDriver(tableConfig);
+ instancePartitions =
driver.assignInstances(InstancePartitionsType.OFFLINE, instanceConfigs, null);
+ assertFilledUniqueAssignment(instancePartitions, instanceConfigs,
numReplicaGroups,
+ numInstancesPerReplicaGroup);
+ }
+
+ /// Builds pool-tagged instance configs whose pool ids start at 1 rather
than 0.
+ private static List<InstanceConfig> newFDAwarePoolInstanceConfigs(int
numInstances, int numPools) {
+ List<InstanceConfig> instanceConfigs = new ArrayList<>(numInstances);
+ for (int i = 0; i < numInstances; i++) {
+ int pool = (i % numPools) + 1;
Review Comment:
The regression helper only generates positive contiguous IDs
(`1..numPools`), although the PR also claims support for negative pool IDs. Add
a minimize-data-movement uplift containing pool `-1`; that case exercises the
existing `INVALID_FD = -1` collision and would catch alive instances being
treated as down.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/assignment/instance/FDAwareInstancePartitionSelector.java:
##########
@@ -324,20 +322,16 @@ private void setNewInstance(int replicaGroupId, int
instanceIndex, Instance inst
Preconditions.checkState(instance.getExistingReplicaGroupId() ==
Instance.NEW_INSTANCE);
_replicaGroupIdToInstancesMap[replicaGroupId][instanceIndex] = instance;
_usedInstances.put(instance.getInstanceName(),
instance.getFaultDomainId());
- _fdCounter[instanceIndex][instance.getFaultDomainId()] += 1;
}
private void setExistingInstance(int replicaGroupId, int instanceIndex,
String instance, int fdId) {
_replicaGroupIdToInstancesMap[replicaGroupId][instanceIndex] = new
Instance(instance, fdId, replicaGroupId);
_usedInstances.put(instance, fdId);
Review Comment:
Negative pool IDs are still unsafe when minimizing data movement.
`reconstructExistingAssignment()` uses `-1` as the missing-fault-domain
sentinel, so an alive instance in pool `-1` is classified as down; during an
uplift it remains in the candidate queue and can move between existing replica
groups. Since this change now allows such tables to be created, distinguish map
absence from the stored ID instead of comparing the ID to the sentinel.
--
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]
