You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Vamsi-klu (via GitHub)" <gi...@apache.org> on 2026/08/15 22:23:27 UTC
[PR] Fix FD_AWARE instance assignment when pool tags do not start at 0 [pinot]
Vamsi-klu opened a new pull request, #19266:
URL: https://github.com/apache/pinot/pull/19266
## Problem
`FDAwareInstancePartitionSelector` sizes `_fdCounter` by the number of pools, then indexes it by the raw Helix pool tag. Deployments whose pool tags are `{1,2,3}` instead of `{0,1,2}` fail table creation:
java.lang.ArrayIndexOutOfBoundsException: Index 3 out of bounds for length 3
at ...FDAwareInstancePartitionSelector$...setNewInstance
`InstanceReplicaGroupPartitionSelector` is fine: it looks pool ids up as map keys, never as array indexes. Existing FD_AWARE tests only tagged pools `0..N-1`, so this never showed up.
## What I did
I deleted `_fdCounter` and the three writes that touched it (`setNewInstance`, `setExistingInstance`, `unSetInstance`).
`_fdCounter` is dead state. It is written in those three places and read nowhere. The only intended consumers, `normalize()` and `swapToInvariantState()`, are still unimplemented stubs. Its only runtime effect today is this crash.
## How I did it
Remove the unused array instead of adding a dense `poolId -> index` map. Net change is about −5 lines in `FDAwareInstancePartitionSelector`. No config, metric, HTTP, wire, or on-disk change. I did not rewrite `InstanceTagPoolSelector`, renumber Helix tags, or implement the `normalize` / `swapToInvariantState` TODOs.
`seekKey` on the minimize-data-movement path still mixes count-space and key-space. The miss is absorbed by `ceilingKey` / `firstKey` (no crash, wrong rotation start). I left that out of this PR on purpose.
If reviewers want the unfinished counter kept for later normalization work, the alternative is a dense map built from the already-sorted `faultDomainToInstanceConfigsMap.keySet()`. Happy to switch.
## Impact
`POST /tables` with `FD_AWARE` and non-zero-based pool tags (`{1,2,3}`, sparse `{5,9}`, single pool `7`, negative pool ids) no longer crashes. Zero-based `{0,1,2}` is unchanged. Replica-group assignment is unchanged.
## Testing
`InstanceAssignmentTest#testPoolBasedFDAwareNonZeroBasedPools` failed on current master with `Index 5 out of bounds for length 5`, then passed after the delete. It copies `testPoolBasedFDAware` but sets `pool = (i % numPools) + 1`, and also covers the reporter `{1,2,3}` case plus an incremental uplift that hits `setExistingInstance` with the raw pool ids.
```
./mvnw -pl pinot-controller -am -Dtest=InstanceAssignmentTest#testPoolBasedFDAwareNonZeroBasedPools,InstanceAssignmentTest#testPoolBasedFDAware test
./mvnw spotless:apply checkstyle:check license:format license:check -pl pinot-controller
```
Fixes #12239
Made with [Cursor](https://cursor.com)
--
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
Re: [PR] Fix FD_AWARE instance assignment when pool tags do not start at 0 [pinot]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: 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
Re: [PR] Fix FD_AWARE instance assignment when pool tags do not start at 0 [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19266:
URL: https://github.com/apache/pinot/pull/19266#issuecomment-5304674204
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/19266?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 100.00%. Comparing base ([`388bc64`](https://app.codecov.io/gh/apache/pinot/commit/388bc64d5c89265ba0b0ab70c2b59af81d3a7862?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e77e0e5`](https://app.codecov.io/gh/apache/pinot/commit/e77e0e523edb8ab95400c225a25bcff0ef681511?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19266 +/- ##
==============================================
+ Coverage 66.97% 100.00% +33.02%
+ Complexity 1423 6 -1417
==============================================
Files 3453 3 -3450
Lines 218949 6 -218943
Branches 34805 0 -34805
==============================================
- Hits 146648 6 -146642
+ Misses 60585 0 -60585
+ Partials 11716 0 -11716
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [java-25](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+33.02%)` | :arrow_up: |
| [lane-a](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
| [lane-b](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+33.02%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/19266/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
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.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/pinot/pull/19266?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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
Re: [PR] Fix FD_AWARE instance assignment when pool tags do not start at 0 [pinot]
Posted by "Jackie-Jiang (via GitHub)" <gi...@apache.org>.
Jackie-Jiang merged PR #19266:
URL: https://github.com/apache/pinot/pull/19266
--
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