Bound the ring walk of ParentConsistentHash::selectParent - #13368
Merged
masaori335 merged 2 commits intoAug 7, 2026
Conversation
When every parent in a consistent_hash pool is down, selectParent walked the whole hash ring taking the global host_status_rwlock on every hop. The ring holds 1024 replica nodes per parent (num_parents * 1024 nodes) and the chash_lookup() gate withholds wrap_around until the ring is traversed twice, so one all-down selection cost ~2 * num_parents * 1024 HostStatus::getHostStatus() calls (~49k for 24 parents) -- inline ET_NET CPU that starved the loopback health probe and drove the VIP flap in inc-p1s2-260703. Track the distinct parents examined on each ring: skip the locked getHostStatus read for a parent already seen, and force wrap_around once every distinct parent has been rejected. The expensive locked read is now paid at most once per parent (O(num_parents)); the ring still advances ~O(N*logN) cheap, lock-free hops to reach every distinct parent. Selection order and the retry-window logic are unchanged. The seen-parent tracking is sized to num_parents (std::vector<bool>), not MAX_PARENTS: the parent.config parser does not cap num_parents at MAX_PARENTS, so a fixed [MAX_PARENTS] array would overflow the stack for pools larger than 64. Add consistent_hash_ring_walk.test.py: an all-down 100-parent pool (marked down via HostStatus, >MAX_PARENTS on purpose) must report "getHostStatus calls: 100", proving the walk reads each parent once instead of walking the full ring.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes ParentConsistentHash::selectParent() for the “all parents down” case by bounding the consistent-hash ring walk so each distinct parent’s HostStatus is read at most once per selection, avoiding excessive contention on the global host-status lock in ET_NET threads.
Changes:
- Add per-selection tracking of “seen” parents to avoid repeated
HostStatus::getHostStatus()calls while walking replica nodes on the hash ring. - Force wrap-around once all distinct parents in the active ring have been rejected, preventing multi-pass full-ring scans.
- Add a gold test that constructs a >64-parent all-down consistent-hash pool and asserts bounded
getHostStatuscall counts.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/proxy/ParentConsistentHash.cc |
Adds seen-parent tracking and bounded ring-walk logic; emits per-selection getHostStatus call count under parent_select debug. |
tests/gold_tests/parent_proxy/consistent_hash_ring_walk.test.py |
New regression test for all-down large pools; validates bounded getHostStatus calls and 502 behavior. |
cmcfarlen
requested changes
Aug 4, 2026
cmcfarlen
left a comment
cmcfarlen
left a comment
Contributor
There was a problem hiding this comment.
Consider using a hybrid stack/heap data structure with reasonable stack size to try to avoid heap allocation in the normal case.
selectParent() runs inline on ET_NET for every transaction, so the two std::vector<bool> allocations per call are pure overhead for what is a 64-flag bitmap in the ordinary case. ts::LocalBuffer<bool, MAX_PARENTS> keeps both rings' flags on the stack (80 bytes each) and falls back to the heap only for a pool larger than MAX_PARENTS, which stays necessary because the parent.config parser does not cap num_parents.
cmcfarlen
pushed a commit
that referenced
this pull request
Aug 9, 2026
* Bound the ring walk of ParentConsistentHash::selectParent When every parent in a consistent_hash pool is down, selectParent walked the whole hash ring taking the global host_status_rwlock on every hop. The ring holds 1024 replica nodes per parent (num_parents * 1024 nodes) and the chash_lookup() gate withholds wrap_around until the ring is traversed twice, so one all-down selection cost ~2 * num_parents * 1024 HostStatus::getHostStatus() calls (~49k for 24 parents) -- inline ET_NET CPU that starved the loopback health probe and drove the VIP flap in inc-p1s2-260703. Track the distinct parents examined on each ring: skip the locked getHostStatus read for a parent already seen, and force wrap_around once every distinct parent has been rejected. The expensive locked read is now paid at most once per parent (O(num_parents)); the ring still advances ~O(N*logN) cheap, lock-free hops to reach every distinct parent. Selection order and the retry-window logic are unchanged. The seen-parent tracking is sized to num_parents (std::vector<bool>), not MAX_PARENTS: the parent.config parser does not cap num_parents at MAX_PARENTS, so a fixed [MAX_PARENTS] array would overflow the stack for pools larger than 64. Add consistent_hash_ring_walk.test.py: an all-down 100-parent pool (marked down via HostStatus, >MAX_PARENTS on purpose) must report "getHostStatus calls: 100", proving the walk reads each parent once instead of walking the full ring. * Keep the parent seen-flags out of the heap in selectParent selectParent() runs inline on ET_NET for every transaction, so the two std::vector<bool> allocations per call are pure overhead for what is a 64-flag bitmap in the ordinary case. ts::LocalBuffer<bool, MAX_PARENTS> keeps both rings' flags on the stack (80 bytes each) and falls back to the heap only for a pool larger than MAX_PARENTS, which stays necessary because the parent.config parser does not cap num_parents. (cherry picked from commit 38076cc)
Contributor
|
Cherry-picked to the 10.2.x branch as c5ea556 for the 10.2.0 release. |
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.
When every parent in a consistent_hash pool is down, selectParent walked the whole hash ring taking the global host_status_rwlock on every hop. The ring holds 1024 replica nodes per parent (num_parents * 1024 nodes) and the chash_lookup() gate withholds wrap_around until the ring is traversed twice, so one all-down selection cost ~2 * num_parents * 1024 HostStatus::getHostStatus() calls (~49k for 24 parents) -- inline ET_NET CPU that starved the loopback health probe and drove the VIP flap in inc-p1s2-260703.
Track the distinct parents examined on each ring: skip the locked getHostStatus read for a parent already seen, and force wrap_around once every distinct parent has been rejected. The expensive locked read is now paid at most once per parent (O(num_parents)); the ring still advances ~O(N*logN) cheap, lock-free hops to reach every distinct parent. Selection order and the retry-window logic are unchanged.
The seen-parent tracking is sized to num_parents (std::vector), not MAX_PARENTS: the parent.config parser does not cap num_parents at MAX_PARENTS, so a fixed [MAX_PARENTS] array would overflow the stack for pools larger than 64.
Add consistent_hash_ring_walk.test.py: an all-down 100-parent pool (marked down via HostStatus, >MAX_PARENTS on purpose) must report "getHostStatus calls: 100", proving the walk reads each parent once instead of walking the full ring.