Fix FD_AWARE instance assignment when pool tags do not start at 0 - #19266
Merged
Jackie-Jiang merged 1 commit intoAug 18, 2026
Merged
Conversation
_fdCounter is sized by pool count but indexed by the raw Helix tag, so
tags such as {1,2,3} crash table creation. The array is written and never
read, so delete it rather than remap the indexes.
Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19266 +/- ##
=========================================
Coverage 66.97% 66.98%
Complexity 1423 1423
=========================================
Files 3453 3453
Lines 218949 218955 +6
Branches 34805 34805
=========================================
+ Hits 146648 146661 +13
- Misses 60585 60586 +1
+ Partials 11716 11708 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jackie-Jiang
requested review from
J-HowHuang
and
a balanced review from Copilot
August 18, 2026 19:53
Jackie-Jiang
added
data-manager
enhancement
Jackie-Jiang
approved these changes
Aug 18, 2026
Jackie-Jiang
added
bug
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes FD-aware assignment crashes for non-zero-based Helix pool IDs.
Changes:
- Removes the unused fault-domain counter indexed by raw pool IDs.
- Adds regression coverage for positive non-zero-based pools and incremental assignment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
FDAwareInstancePartitionSelector.java |
Removes unsafe dead counter state. |
InstanceAssignmentTest.java |
Tests non-zero-based pool assignment. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
327
to
329
| private void setExistingInstance(int replicaGroupId, int instanceIndex, String instance, int fdId) { | ||
| _replicaGroupIdToInstancesMap[replicaGroupId][instanceIndex] = new Instance(instance, fdId, replicaGroupId); | ||
| _usedInstances.put(instance, fdId); |
| 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; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
FDAwareInstancePartitionSelectorsizes_fdCounterby 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:InstanceReplicaGroupPartitionSelectoris fine: it looks pool ids up as map keys, never as array indexes. Existing FD_AWARE tests only tagged pools0..N-1, so this never showed up.What I did
I deleted
_fdCounterand the three writes that touched it (setNewInstance,setExistingInstance,unSetInstance)._fdCounteris dead state. It is written in those three places and read nowhere. The only intended consumers,normalize()andswapToInvariantState(), 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 -> indexmap. Net change is about −5 lines inFDAwareInstancePartitionSelector. No config, metric, HTTP, wire, or on-disk change. I did not rewriteInstanceTagPoolSelector, renumber Helix tags, or implement thenormalize/swapToInvariantStateTODOs.seekKeyon the minimize-data-movement path still mixes count-space and key-space. The miss is absorbed byceilingKey/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 /tableswithFD_AWAREand non-zero-based pool tags ({1,2,3}, sparse{5,9}, single pool7, negative pool ids) no longer crashes. Zero-based{0,1,2}is unchanged. Replica-group assignment is unchanged.Testing
InstanceAssignmentTest#testPoolBasedFDAwareNonZeroBasedPoolsfailed on current master withIndex 5 out of bounds for length 5, then passed after the delete. It copiestestPoolBasedFDAwarebut setspool = (i % numPools) + 1, and also covers the reporter{1,2,3}case plus an incremental uplift that hitssetExistingInstancewith the raw pool ids.Fixes #12239
Made with Cursor