fix(inkless:switch): make consolidating replicas wait for ISR at the seal - #760
fix(inkless:switch): make consolidating replicas wait for ISR at the seal#760jeqo wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes ISR recovery for consolidating replicas during classic-to-diskless handoff.
Changes:
- Requires ISR membership before evicting a classic replica fetcher.
- Adds regression coverage for consolidating replicas outside ISR.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ReplicaFetcherThread.scala |
Keeps out-of-ISR replicas fetching at the seal. |
ReplicaFetcherThreadTest.scala |
Adds consolidating-replica eviction coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
2157049 to
8ce895f
Compare
834c09a to
64c56db
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
docs/inkless/CLASSIC_TO_DISKLESS_SWITCH.md:133
- The following reconciliation bullet still says every local tail above the seal is truncated, but this PR deliberately preserves a consolidating suffix when its diskless epoch validates. Update that bullet so the switch documentation doesn't contradict the new past-seal recovery behavior.
Followers also seal during the switch. While the switch is pending (`-2`), they keep using the classic fetcher to replicate the frozen classic prefix. After the final start offset is committed, followers continue until their local LEO reaches the committed seal offset. A follower that reached the seal while outside ISR keeps fetching until the leader admits it back, so a replica never leaves the classic prefix without having proven it holds all of it.
core/src/main/scala/kafka/cluster/Partition.scala:1041
- Consolidation materializes diskless records at and above the seal in local logs, including on the leader used by this method. Calling the classic prefix the only records that live locally contradicts the behavior this PR handles and obscures why seal-based admission is safe.
* and never joins the ISR. Reaching the seal is the whole evidence a switched partition needs: the
* records below it are the only ones that live in local logs.
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala:232
UnderReplicatedPartitionsexplicitly excludes consolidating partitions (ReplicaManager.scala:468-470), so this claims an operational signal that doesn't exist for the stuck state motivating the warning. Document that the warning supplies the missing partition-level signal instead.
* Warns about partitions that have waited a long time for ISR readmission at the seal. Readmission
* normally lands within a fetch cycle or two, and the only other symptom is a non-zero
* UnderReplicatedPartitions, which names neither the partition nor the reason. Note that
* isReplicaInIsr also returns false when the metadata image holds no entry for the topic, so a
* stale image is indistinguishable from a genuine ISR miss here.
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala:252
- This wait path also handles switched topics that don't use consolidation, so those replicas receive a warning that incorrectly says a consolidation handoff is blocked. Make the final sentence describe the shared behavior: the replica remains on the classic fetcher until ISR admission.
warn(s"$tp has waited ${now - since} ms at the classic-to-diskless seal $seal (local log end " +
s"offset $leo) for the leader to admit this replica back to ISR. The hand-off to " +
s"consolidation stays blocked until then.")
core/src/test/java/kafka/server/InklessConsolidatedDisklessTopicsTest.java:319
createClassicTopicleaves placement toStripedReplicaPlacer, which randomizes the first replica, so broker 1 isn't guaranteed to be the follower. This assertion makes the regression test fail nondeterministically before it exercises recovery. Create this partition with an explicit[0, 1]replica order so broker 1 is always the broker-only follower.
final int restartedBrokerId = 1;
assertTrue(Arrays.stream(initial.replicas).anyMatch(id -> id == restartedBrokerId),
"Broker 1 must host the partition so the test can restart the broker-only node");
assertTrue(initial.leader != restartedBrokerId,
"Broker 1 must be the follower so the test exercises follower recovery without a leader change");
64c56db to
9b28b76
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala:243
- This pruning doesn't clear timestamps when
ReplicaFetcherManagerremoves a partition externally. If the same topic-partition is re-added to this fetcher before another wait invokes this method,fetchState(tp)is defined again and the old timestamp survives, causing an immediate warning with elapsed time from a previous fetcher residency. Clear both timestamp maps on every partition removal, not only the self-eviction path.
isrRecoveryWaitStartMs.filterInPlace((tp, _) => fetchState(tp).isDefined)
isrRecoveryLastWarnMs.filterInPlace((tp, _) => isrRecoveryWaitStartMs.contains(tp))
docs/inkless/CLASSIC_TO_DISKLESS_SWITCH.md:138
AGENTS.md:139-142requires list items that contain verbs to start with a capital letter and end with a period. Apply that formatting to both revised items.
* if local LEO is above the seal, it preserves a consolidated suffix only when the local epoch cache proves that the suffix belongs to the captured diskless epoch; otherwise it truncates to the seal
* if a leader's local LEO is below the seal, it stays online to rebuild the classic prefix when consolidation and remote storage are available; otherwise it marks the partition offline
364ac3d to
08edd6e
Compare
A restarted replica of a switched consolidating partition can recover a local log past the classic-to-diskless seal and go straight back to consolidation. The consolidation fetcher reads object storage and never sends the leader the follower fetch needed for ISR readmission, so the replica can stay outside ISR indefinitely while continuing to look healthy. Reproduce that lifecycle with a real two-broker cluster: create a classic partition, switch it to consolidated diskless, require both brokers to consolidate past the seal, restart the broker-only follower, and assert that the controller ISR shrinks and returns to full size. Also verify that the recovered follower preserves its validated diskless suffix and that the complete classic prefix plus diskless suffix remains readable. Read ISR from the brokers' KRaft metadata image rather than DescribeTopics. Inkless transforms client-facing ISR from broker liveness, so DescribeTopics can report the restarted broker as in sync even when the controller never readmits it, making the regression test pass vacuously. Verified red on main: KRaft ISR remains leader-only after the follower restarts. Verified green with the recovery fix.
…seal A replica outside ISR must keep fetching from the leader until the leader has observed the catch-up. The leader records the offset carried by the fetch request, not the log end offset after the append, so it sees the replica at the seal only on the following fetch. partitionsAwaitingIsrRecovery exists to provide that fetch. The eviction check short-circuited on isConsolidatingPartition and skipped the wait. Once a consolidating partition hands off, the consolidation fetcher reads object storage and sends no fetch to the leader, so nothing else expands ISR. Since #754 also stops the controller from expanding ISR for switched partitions, a consolidating switched replica outside ISR had no admission path at all. This commit covers a replica that arrives at the seal from below; a restarted replica never reaches this fetcher at all, which the routing change later in this branch fixes. Drop the short-circuit. It only changed behavior for a replica outside ISR, which is the case that needs the wait; a replica already in ISR still evicts on isReplicaInIsr and hands off unchanged. The pending backoff belongs to one fetcher residency. Partition removal can run on the metadata publisher thread, so remove the pending entry under partitionMapLock and drain the buffer under the same lock. Fetcher managers hold their monitor before calling removePartitions, so never call replicaFetcherManager or consolidationFetcherManager while holding partitionMapLock; that would invert the manager-to-partition order. This matters now because diskless.remote.storage.consolidation.enable requires diskless.allow.from.classic.enable, so any cluster that enables consolidation can switch topics, and the switch auto-enables remote storage on the topic. Every switched partition in such a cluster is consolidating. Testing shouldDelayConsolidatingPartitionAtSealWhileOutsideIsr asserts the partition is queued in partitionsAwaitingIsrRecovery, not evicted, then that the back-off is applied with replica.fetch.backoff.ms once doWork drains the queue. It is standalone rather than routed through verifyDisklessSwitchEviction, which proves only the absence of eviction: that helper never adds the partition to the fetcher, so delayPartitions is a silent no-op, and doWork clears the queue before the helper inspects it. Verified red-then-green: restoring the short-circuit fails the queueing assertion. shouldClearPendingIsrRecoveryBackoffWhenPartitionIsRemoved removes and re-adds a partition before the pending buffer drains. The new residency remains immediately fetchable instead of inheriting the old residency's delay.
…ranch The consolidating branch of fetchMessages is not gated on isFromConsumer, so a follower fetch enters it too. On a consolidating leader the local log end offset is the consolidated frontier, well past the seal, so a follower fetch at the seal satisfies fetchOffset < logEndOffset and reads the consolidated suffix from the leader's local log. That skips the at-seal branch entirely and contradicts the invariant stated in the same method: followers must never replicate diskless records into their local log. It also ships those records over the inter-broker path, billing them to ReplicationBytesInPerSec rather than ConsolidationFetchBytesInPerSec, which is the traffic consolidation exists to avoid, and defers ISR admission until the follower matches the frontier. Exclude a follower fetch at or above a committed seal from the local-log path, so the at-seal branch answers it with empty records and the high watermark clamped to the seal. The exclusion is inert today: a consolidating follower evicts from the classic fetcher on the same call that brings it to the seal, so it never issues a fetch at or above the seal. It becomes load-bearing as soon as a consolidating follower stays on the classic fetcher at the seal, which both the ISR wait in this branch and the leader-change diversion in #756 introduce. Testing testFollowerFetchAtSealOnConsolidatingLeaderGetsAtSealResponse appends past the seal on the leader, then asserts a follower fetch at the seal receives empty records with the high watermark at the seal, and that the branch still records the follower's position. Without the exclusion the follower receives 85 bytes of the leader's consolidated log.
…ISR admits it isReadyForConsolidation routes on offsets alone, so a follower whose local log already covers the seal goes straight to the consolidation fetcher. That fetcher reads object storage through DisklessLeaderEndPoint and sends no fetch to the leader, so the leader never observes the catch-up and the AlterPartition handshake that expands ISR never starts. Since #754 also stops the controller expanding ISR for switched partitions, such a replica has no admission path. A broker restart reaches this state without any leader change: controlled shutdown removes the replica from ISR, the restart recovers a log end offset at or beyond the seal, and the routing hands the partition to consolidation before it can prove itself. A running broker can reach the same state after fencing: the ISR-only delta advances the partition epoch but not the leader epoch, so applyLocalFollowersDelta must explicitly move the existing consolidation fetcher back to the classic fetcher. Stopping both fetcher managers before the restart keeps that hand-off exclusive. Without the hold-back and the live reroute, the replica stays out of ISR for the rest of the partition's life, and a rolling restart reaches every partition of a switched topic. Nothing reports it: underReplicatedPartitionCount excludes consolidating partitions, so no metric moves, and the state surfaces only when a leader election needs an in-sync replica and finds the leader alone. The ISR wait added earlier in this branch cannot help either, because the partition never reaches the classic fetcher where that wait lives. Hold back every follower at or above the seal while it is outside ISR. The classic fetcher waits there until the leader admits it back to ISR, then evicts and hands off as it already does for a follower that arrives from below the seal. Admission needs the seal-gated predicate added later in this branch: recording the position alone cannot satisfy the generic in-sync check on a consolidating leader, whose local high watermark sits at the diskless frontier. A replica that already consolidated past the seal keeps its diskless suffix while it waits. initialFetchOffset starts the classic fetcher at the real log end offset, and the leader answers from the at-seal branch rather than its local log, so nothing immediately overwrites the suffix. The later admission commit validates that state against a leader that holds the complete classic prefix. If that leader cannot resolve the diskless epoch, standard out-of-range recovery may discard only the post-seal suffix. A leader below the seal is different: it cannot safely validate or truncate the follower until it rebuilds its own classic prefix, which the admission commit also guards. Restricting the hold-back to log end offset == seal would have been narrower and wrong. A follower that consolidated anything at all sits above the seal, so the common rolling-restart case would still route to consolidation and still have no way back into ISR. Testing Four tests pin the routing decision and the live reroute: testApplyDeltaKeepsConsolidatingFollowerAtSealOnClassicFetcherWhileOutsideIsr asserts the classic fetcher receives the partition with its initial offset at the seal. It captures the argument rather than counting invocations, so an empty map cannot satisfy it. Without the hold-back the classic fetcher receives nothing. testApplyDeltaKeepsConsolidatingFollowerPastSealOnClassicFetcherWhileOutsideIsr covers a replica already consolidated past the seal: it asserts the classic fetcher takes it at the real log end offset, not at the seal, and that the local log still ends where it did. Restoring the seal equality fails only this test. testApplyDeltaSendsConsolidatingFollowerAtSealToConsolidationWhileInIsr covers the same state with the replica in ISR. Dropping the ISR term from the condition parks an in-ISR replica on the classic fetcher permanently, and fails only this test. testApplyDeltaReroutesConsolidatingFollowerToClassicFetcherWhenRemovedFromIsr applies an ISR-only delta with the same leader epoch and a higher partition epoch. It asserts that the partition starts on consolidation, both fetcher managers remove it, and the classic fetcher restarts at the existing log end offset.
…eader high watermark The at-seal fetch records the follower's position and then relies on the generic ISR expansion that updateFetchState triggers. That expansion gates on isFollowerInSync, which compares the recorded offset against this leader's local high watermark. For a non-consolidating switched partition the leader's log is frozen at the seal, so the comparison is seal >= seal and the follower joins ISR. A consolidating leader runs its own consolidation fetcher, and ReplicaFetcherThread.processPartitionData calls maybeUpdateHighWatermark with the diskless high watermark, which sets the local watermark directly and bypasses maybeIncrementLeaderHW. The leader's high watermark is therefore the consolidated frontier. The at-seal branch deliberately clamps the recorded follower offset to the seal, because a follower must never replicate diskless records over the inter-broker path, so the comparison becomes seal >= frontier and is never true. The follower never joins ISR. With the hold-back earlier in this branch that is worse than leaving the short-circuit in place: the follower stays on the classic fetcher, so it is both under-replicated and never consolidating, and its local log stays pinned at the seal. Admit on the seal instead. A successfully validated fetch at the seal is sufficient evidence when the leader itself holds the complete classic prefix: records below the seal are the only ones replicated from this leader. maybeExpandIsrAtSeal mirrors maybeExpandIsr, replacing the high-watermark comparison with the seal comparison and keeping the leadership, eligibility, in-flight, locking, and submission safeguards. A consolidating leader below the seal is rebuilding a lost classic prefix from the remote tier. It has no authority to validate an intact follower against its own incomplete epoch cache: OFFSET_OUT_OF_RANGE or a diverging epoch would make the follower discard the prefix the leader lacks. While the leader is below the seal, the at-seal branch therefore returns the non-destructive empty response but performs neither validation nor ISR admission. The follower keeps fetching with backoff until the leader has rebuilt through the seal. The diverging-epoch guard is load-bearing. Partition.fetchRecords skips updateFollowerFetchState on divergence and preserves any position recorded by an earlier fetch. Admission must therefore depend on this fetch validating successfully, or stale at-seal evidence could admit a now-divergent follower. Seal-based admission also intentionally drops isFollowerInSync's current-leader-epoch-start requirement: post-seal records live in shared object storage and are not replicated from this leader, so holding data in that epoch is not part of the follower's classic-prefix proof. Logical retention can advance the leader's log start offset beyond the seal after the classic prefix expires from the authoritative range. At that point the partition is equivalent to born-diskless: there is no retained classic range whose lineage can gate ISR membership. Validating at the fixed seal would return OFFSET_OUT_OF_RANGE, while validating at the retained start would report a false divergence for an at-seal follower carrying the classic epoch. Handle that expired-prefix state explicitly. Validate leadership, assignment, leader epoch, and broker epoch, but skip lineage for the expired range. Record the follower's actual requested offset rather than the leader's retained start, then admit through the same seal predicate. This neither invents follower progress nor lets the synthetic position affect the leader high watermark. Testing testFollowerFetchAtSealOnConsolidatingLeaderGetsAtSealResponse models the leader's high watermark at the diskless frontier and asserts an AlterPartition carrying the follower. Without the frontier watermark the test passed while the production path could not expand ISR, which is how the chain first reached review broken. testFollowerAtSealWaitsWhileConsolidatingLeaderRebuildsClassicPrefix leaves the leader's classic epoch ending below the seal and sends that epoch in the follower fetch. It asserts the follower receives no diverging epoch and no AlterPartition while the leader is incomplete. Without the leader-LEO guard the response carries a silent diverging epoch that makes the follower truncate its complete prefix. testFollowerFetchPastSealCarryingDisklessEpochIsAdmittedWhenLeaderHasConsolidated asserts a fetch above the seal carrying an epoch the leader can resolve reports no divergence and produces an AlterPartition with the suffix intact. testFollowerFetchPastSealSignalsOutOfRangeWhenLeaderLacksTheDisklessEpoch covers a leader at the seal that cannot resolve the follower's post-seal epoch: the response is OFFSET_OUT_OF_RANGE and no AlterPartition is submitted, sending the follower through standard suffix recovery without risking the classic prefix. testFollowerAtSealIsAdmittedAfterLeaderRetentionPassesSeal gives the leader the real two-epoch layout - a classic epoch below the seal and the captured diskless epoch above it - then advances logical log start beyond the seal. An at-seal follower carrying the classic epoch is admitted without a false divergence, its recorded position remains the actual seal, and the response stays empty and clamped to the seal. testFollowerFetchAtSealIsolatesUnavailableLocalLog makes one online partition throw the NotLeaderOrFollowerException that localLogOrException uses when its log is absent, while a sibling validates normally. It asserts the failed partition gets its own error without preventing the sibling from recording its follower and submitting AlterPartition. Keeping the log lookup inside the per-partition try preserves that isolation.
Five comments still described the classic fetcher as self-evicting at the seal, which was true until this branch made the hand-off wait for ISR readmission. The one at the at-seal fetch response is the worst of them: it sits on the only admission path and says a consolidating partition evicts immediately, so someone trusting it would delete that path. The at-seal comment also credited ISR expansion to the generic expansion that updateFetchState triggers. Admission now runs through maybeExpandIsrAtSeal, and the recording it describes only keeps replica state current. The isReadyForConsolidation doc also still restricted the wait to a log end offset exactly at the seal, which the preceding commit widened to at or above it.
08edd6e to
82c0e1a
Compare
A restarted replica of a switched consolidating partition can remain outside the controller ISR indefinitely. The replica has the complete classic prefix and may have consolidated beyond the switch seal, but the consolidation fetcher reads object storage locally and sends no follower FETCH to the Kafka leader. Without that fetch, the leader never receives the evidence required to readmit the replica.
Affected invariant
A switched follower may leave the classic fetcher only after the leader has observed a validated follower fetch at or above the classic-to-diskless seal and admitted the follower to ISR.
This differs from a born-diskless partition. A switched partition still has a classic prefix that replicas originally copied from the Kafka leader. Broker liveness and
LEO >= sealdon't prove to the current leader that a replica holds that prefix. The leader-observed follower fetch is the proof.The invariant has four parts:
Regression
The regression appears after an ordinary broker restart:
AlterPartitionthat would restore ISR membership.The same state is reachable without restarting the JVM. Fencing removes a running follower from ISR and advances only the partition epoch. The old code restarted its fetcher only on a leader-epoch change, so the existing consolidation fetcher remained attached and the follower again had no readmission path.
Integration baseline
testSwitchedConsolidatingFollowerRejoinsIsrAfterRestartreproduces the broker lifecycle with a real two-broker cluster, PostgreSQL control plane, object storage, switching, and consolidation:The test reads ISR from the brokers' KRaft metadata image rather than
DescribeTopics. Inkless transforms client-facing ISR from broker liveness, soDescribeTopicscan report the restarted broker as in sync even when the controller never readmits it. Using client-facing metadata made the first version of the test pass vacuously.The integration test is red on
main: after the follower restarts, KRaft remains atisr=[leader]. It is green with this branch.Failure on
mainThe baseline times out waiting for the restarted follower to rejoin ISR. The final KRaft registration remains:
Broker
0is running, still assigned, and has consolidated beyond the seal, but the controller never readmits it. The stuck state is therefore not a broker-startup failure or missing data: it is a live assigned replica using the consolidation fetcher with no leader-observed follower state and no path back into the controller ISR.The immediate production symptom is weak:
DescribeTopicscan still show the restarted broker as in sync because Inkless rebuilds client-facing ISR from broker liveness.The damage is deferred. A rolling restart repeats the same transition for each replica and can leave KRaft with only the current leader in ISR. A later leader movement or failure then has fewer controller-approved replicas available and can expose the incomplete recovery as a leader-election or classic-prefix availability problem. On
main, operators receive no partition-level warning that the replica is stuck in this state.How the commits build the solution
The integration test lands first as the regression baseline. The next four commits form one recovery chain; none is sufficient alone.
ReplicaFetcherThreadno longer lets consolidation bypass the ISR check at the seal. The extra fetch is necessary because the leader records the offset carried by the request, not the follower LEO after append.isReadyForConsolidationnow requires ISR membership, not onlyLEO >= seal. This covers followers exactly at the seal, followers already consolidated past it, restarted followers, and running followers removed by an ISR-only metadata delta. Both fetcher managers remove the partition before the selected fetcher starts, keeping the handoff exclusive.Partition.maybeExpandIsrAtSealpreserves the normal ISR-update safeguards but replaces the generic high-watermark predicate with the switch boundary. A consolidating leader advances its local high watermark into the diskless suffix, which the follower must not replicate from that leader, so generic admission can never succeed.After the controller commits the ISR expansion, the classic fetcher observes the new membership, evicts the partition, and starts consolidation again.
The final two commits align the switch documentation with this handoff contract and add a once-per-minute warning when a replica remains at the seal waiting for readmission. The warning is the direct operator signal for this state because the aggregate under-replicated metric excludes consolidating partitions.
See the individual commit messages for the reasoning, rejected narrower conditions, and mutation checks behind each step.
Past-seal recovery
A restarted follower may already have a diskless suffix and
LEO > seal. The classic fetcher starts at the real local LEO rather than truncating eagerly:OFFSET_OUT_OF_RANGEand standard follower recovery truncates to the leader's frontier. No ISR update is submitted before that recovery.Restricting recovery to
LEO == sealleaves the normal steady-state restart broken, because any replica that has consolidated records starts above the seal.Operator impact
No config, metric, or protocol changes.
A new warning identifies a partition that remains on the classic fetcher waiting for ISR readmission. It reports the elapsed wait, seal, and local LEO after one minute and then once per minute. Normal recovery completes in a few fetch cycles and remains silent.
Testing
The integration baseline fails on
mainwith the restarted follower absent from KRaft ISR and passes on this branch. The unit suites cover fetcher routing at and past the seal, ISR-only partition-epoch changes, both diskless-epoch validation outcomes, seal-based admission with leader HW beyond the seal, fetch backoff, warning timing, and handoff cleanup.