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: [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]
