feat(sharding): place regions and primaries onto joining nodes - #115
Open
austin-barrington wants to merge 9 commits into
Open
austin-barrington wants to merge 9 commits into
austin-barrington wants to merge 9 commits into
Conversation
Bootstrap now takes RF = min(configured, live members), so n=1 gets peers=[self] even when configured RF is 3.
A joiner's map_version must match the cluster's before any peer-set change, so staging cannot run against a lagging epoch.
A joining node became a member but never received data for measurements that already existed: `try_split` clones the parent peer set and `try_rebalance` only moves a primary among peers already in the region, so an existing region's peer set never grew to include a new process. Add `ShardMapOp::AddPeer` and a scheduler placement step that stages the region onto a live Active member before committing it as a peer. Staging first is the contract here — the heal path's commit-then-stage ordering would publish a peer that holds no rows. Apply refuses `AddPeer` on a region carrying outstanding transfer debt, on a duplicate peer, and on a stale epoch. `ShardRehomeRequest.stage` lets the scheduler ask a remote primary to run the same staging push when the leader is not the region's primary; the rehome handler skips `complete_region_transfer` for a staging push since ownership does not change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Te8YUXjjLssk3hxwoE5Db
A joiner that received region data was still only a replica: nothing moved a primary onto it, so writes kept landing on the original owner and the new process added HA but no write capacity. Add two placement steps to the scheduler tick, both stage-then-commit so an empty node never owns a range: - `try_place_primary` hands a region's primary to its newest peer while that peer carries strictly fewer primaries than the current one. The "newest peer" bias is what makes it terminate — once the joiner is primary the rule no longer applies, so ownership cannot oscillate back, and the count guard stops every region stampeding onto one node. - `try_place_idle_member` covers 3->4, where no region is under RF and `AddPeer` correctly declines. An existing replica steps aside for the newest member via `MovePeer`, which appends it and so makes it the next primary-placement candidate. `try_rebalance` scored a peer with no heartbeat row as 0 bytes, which made any freshly staged replica look infinitely lighter than the primary and handed it ownership on the strength of missing telemetry. Unmeasured peers are now skipped rather than treated as empty. Integration coverage asserts the contract rather than the midpoint: a write aimed at the replica-only joiner forwards and leaves its WAL empty, and the same write applies locally once the primary has moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Te8YUXjjLssk3hxwoE5Db
Both placement steps ran against the region as it looked when the tick snapshotted the map. A placement earlier in the same tick invalidates that: `AddPeer` bumps the epoch and appends a peer, so the next step would stage an entire region copy and only then have its proposal rejected by the epoch CAS — wasted transfer, repeated every tick until the snapshots happened to line up. Re-read the region from a fresh snapshot inside each step, and skip it if the region has since been split, merged, or dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Te8YUXjjLssk3hxwoE5Db
Seven findings from reviewing the branch, four of which could corrupt data or wedge a cluster. **Rollup destinations were eligible for placement.** `apply_transfer_push` has no idempotence guard, so a region staged onto a joiner whose map op then fails is staged again next tick. `ReplacingMergeTree` measurements collapse the redelivery; `SummingMergeTree` rollup destinations sum it and are corrupted permanently. `enqueue_reconciliation` already excluded these spaces for exactly this reason — all three placement paths now share that guard, failing closed when the lock is contended. **`AddPeer` had no upgrade gate.** `ClusterRequest` is serialised into the Raft log, so a committed op an un-upgraded voter cannot deserialise wedges it. `ClearVerified` set the precedent; `AddPeer` now has the same gate and defaults **off** for the release that introduces it. **`sync_shard_map` could move the map backwards.** The sync peer is whichever Active node came first out of a HashMap — not the leader, not necessarily the most-applied node. Installing an older snapshot dropped ops Raft never redelivers, because `replace_map` bypasses the state machine and leaves `last_applied` untouched; every later op then failed StaleEpoch and the node silently stopped owning regions it held data for. Install only a strictly newer map. **A missing shard map aborted the whole join.** `/internal/shard/map` only exists on peers with sharding enabled, so a 404 from an older or unsharded peer failed `join_and_sync` *before* WAL catch-up; after the retries the node went Active having synced nothing. It is best-effort now: with no map there are no regions to move, and the leader's own catch-up check still gates placement onto this node. Also: `try_place_live_member` never re-read its region, so it staged a full copy before failing the epoch CAS (the sibling fix missed it); and the steps after a placement ran on the region as it looked *before* that placement committed — `try_rebalance` would spend an entire region copy on a stale peer list. A placement that commits now yields the region. Placement was reachable only from `tick()` and no test called it, so deleting those calls left the suite green. Adds a harness with a mock peer that drives a real tick, covering both the placement and the upgrade gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Te8YUXjjLssk3hxwoE5Db
Placement could stop cluster-wide, permanently and quietly. `joiner_map_caught_up` required exact `map_version` equality and `live_replica_candidate` always returned the lowest-id non-peer, so a single node that never matched was chosen first, failed the probe, and was chosen again the next tick. No other candidate was ever tried, for any region, and the only trace was a `debug!` line. - Catch-up accepts a candidate that is *ahead*. The cluster version comes from a snapshot read just before the probe, so a node that applied an op in between is legitimately ahead and was being rejected for it. Behind is still refused — that is what the gate is for. - `live_replica_candidates` returns every eligible member in id order and placement takes the first that passes its probe, instead of stopping at a candidate chosen before any probe ran. - A region that goes `PLACEMENT_STALL_WARN_TICKS` ticks with candidates but none caught up now warns and increments a counter. A stall that only shows up at debug level is a stall nobody finds. - `/internal/shard/map` serialises every space and region to answer one `u64`, and it was probed once per region per tick — O(regions x map size) bytes on every tick of a join. Probes are cached per peer for the tick. Also corrects the `handle_shard_transfer` staging comment, which still claimed a local-measurement guard that P1.3 deliberately removed. H.6 (whether the gate should compare the Raft applied index instead) is answered in the plan, no code change: the index is already exposed at /internal/raft/metrics, but `replace_map` installs a map without advancing it, so an applied-index gate would reject exactly the joiner it should admit. Fix belongs with Phase 2's snapshot-based map recovery. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Te8YUXjjLssk3hxwoE5Db
`try_place_idle_member` was documented as giving "a member that joined an already-replicated cluster" a replica slot, which reads as join-triggered. It is not. The target is the active member with the greatest `joined_at` — whoever joined last, with no recency window, so "last" may mean months ago. The rule fires whenever that member is under-loaded against some region's non-primary peer, which means a stable-but-unbalanced cluster starts moving replicas on the first tick after an upgrade rather than on any join. The behaviour is deliberately unchanged. It is bounded and convergent, and every copy is staged and verified before the map changes, so this is a scheduling surprise rather than a correctness risk — acceptable while sharding is beta. Gating it was considered and rejected for now; the doc says so, and says to revisit before sharding graduates. Renames `newcomer` to `latest_member` throughout, so the identifier stops implying recency too. No logic change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Te8YUXjjLssk3hxwoE5Db
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_04f83f13-d9f9-4015-a75d-535c538c47b0) |
PR SummaryCursor Bugbot is generating a summary for commit e96f7a0. Configure here. |
Shipped it off, which was the wrong call. The gate is an opt-out for a rolling upgrade, not an opt-in for the feature — `transfer_clear_proposals_ enabled` guards the identical hazard and defaults on, and defaulting off leaves region placement inert for everyone who enables sharding and never finds the flag. The hazard is bounded: a follower on a build that does not know `AddPeer` rejects the whole append RPC (axum fails the body before the handler runs), so it stops replicating and the leader can lose commit quorum for the duration. It resolves as soon as every node is upgraded, and ingest does not go through Raft. Operators disable the gate for the rollout window. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019Te8YUXjjLssk3hxwoE5Db
This was referenced Sep 7, 2026
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.
Sharding partitions each measurement's
series_idspace into regions, but aprocess joining an existing cluster never received any of them.
try_splitclones the parent peer set and
try_rebalanceonly moves a primary amongpeers already in the region, so an existing measurement's peer set could not
grow to include a new node. A joiner added membership and HA, and no write
capacity.
This is Phase 1 of first-class sharding: a one-member cluster owns regions, a
joining node receives existing region data, and then a primary moves onto it
so writes land there locally.
What changes
min(configured, live members). A 1-member cluster getspeers = [self]instead of an impossible peer set at configured RF 3.staging can never run against a lagging epoch (
ShardMapPort::replace_map).ShardMapOp::AddPeergrows a region's replica set. Apply refuses aduplicate peer, a stale epoch, and any region carrying transfer debt.
try_place_live_member— stages a region onto a live member while theregion is below effective RF, then commits
AddPeer.try_place_idle_member— covers 3→4, where no region is under RF andAddPeercorrectly declines; an existing replica steps aside viaMovePeer.try_place_primary— hands the primary to the region's newest peer whilethat peer holds strictly fewer primaries than the incumbent.
Staging before committing is the contract here. The heal path's
commit-then-stage ordering would publish a peer holding no rows, and scatter
reads fall back to replicas, so that surfaces as silent empty results.
Primary placement terminates by construction rather than by cooldown. Pure
count balancing cannot satisfy the goal: with one region and two members,
moving the only primary does not improve balance, it swaps which node is idle
— so a balancer either refuses to move or oscillates forever. Keying on the
region's newest peer means the rule stops applying the moment that peer is
primary, and the count guard stops every region stampeding onto one joiner.
Reviewer note: rolling upgrades
AddPeeris a newShardMapOpvariant andClusterRequestis serialised intothe Raft log. A node on a build that does not know the variant fails to
deserialise the whole append RPC — axum rejects the body before the handler
runs — so it stops replicating, and a leader mid-rollout can lose commit
quorum on the control plane. It resolves as soon as every node is upgraded,
and ingest does not go through Raft.
add_peer_proposals_enableddefaults on, matchingtransfer_clear_proposals_enabled, which guards the identical hazard. The flagis an opt-out for the duration of a rolling upgrade, not an opt-in for the
feature. Disable it before a rollout of a sharded cluster; re-enable when every
node is on the new build.
Review fixes included
The branch was reviewed before this PR; 14 findings, 12 fixed here.
Correctness:
SummingMergeTree) destinations were eligible for placement.apply_transfer_pushis not idempotent, so a region staged onto a joinerwhose map op then failed was staged again next tick and summed twice —
permanent silent corruption. All three paths now share the guard
enqueue_reconciliationalready used, failing closed on lock contention.sync_shard_mapcould install an older peer map. The sync peer iswhichever Active node came first out of a
HashMap, not the leader; anolder map dropped ops Raft never redelivers, because
replace_mapbypassesthe state machine and leaves
last_applieduntouched. Strictly-newer only./internal/shard/mapaborted the whole join before WALcatch-up, and after the retries the node went Active having synced nothing.
Best-effort now: with no map there are no regions to move anyway.
required exact
map_versionequality while the candidate was chosen beforeany probe and never advanced, so one lagging node blocked every region
forever with only a
debug!line. Catch-up now accepts a candidate that isahead, placement walks candidates, and a persistent stall warns and
increments
hyperbytedb_shard_placement_stalled_total.try_rebalancescored a peer with no heartbeat row as 0 bytes. Unmeasuredis not empty — a freshly staged replica looked infinitely lighter than the
primary and would have taken ownership on missing telemetry.
in the same tick made later steps spend a full region copy before failing
the epoch CAS.
Efficiency:
/internal/shard/mapserialises every space and region to answer oneu64and was probed once per region per tick. Cached per peer per tick.
Testing
cargo test --lib— 521 passcargo test --test '*'— 288 pass across 11 suitescargo test -p hyperbytedb-cli— 38 passcargo clippy --all-targets -- -D warnings,cargo fmt --check— cleanIntegration coverage asserts the contract, not the midpoint: a write aimed at
a replica-only joiner forwards and leaves its WAL empty, and the same write
applies locally once the primary has moved. The tick wiring, the stall skip
and the probe cache are each mutation-tested — reverting them reproduces the
original bug rather than merely failing.
+2570 / −59, of which +846 is tests.
Supersedes
Closes the earlier stack, whose commits are the first two on this branch:
feat/sharding-n1-legal-cluster(f7b550a)feat/sharding-joiner-map-catchup(8bdbd99)Not in this PR
remains experimental and opt-in.
pull, not by reconstruction after log purge. Related: whether the catch-up
gate should compare the Raft applied index instead of
map_version— it isalready exposed at
/internal/raft/metrics, butreplace_mapinstalls amap without advancing it, so an applied-index gate would reject exactly the
joiner it should admit. Belongs with the Phase 2 snapshot work.
gate has not been built; placement is proven by in-process tests only.
application layer, and peer RPC is raw HTTP there. Planned separately; this
PR adds no new
use crate::adaptersimports toapplication/.try_place_idle_memberis not join-triggered. It targets the memberwith the greatest
joined_at, so a stable-but-unbalanced cluster rebalanceson the first tick after upgrade. Bounded, convergent, verified before
commit; documented and accepted while sharding is beta.
Companion: hyperbytedb-operator hyperbyte-cloud/hyperbytedb-operator#15
drops the webhook rule forbidding
sharding.enabledatreplicas: 1andemits
[cluster] enabled = truefor a 1-replica CR with explicit sharding.Merge order does not matter; neither side depends on the other at runtime.
🤖 Generated with Claude Code
https://claude.ai/code/session_019Te8YUXjjLssk3hxwoE5Db