fix(inkless:switch): clear under-replicated partitions after classic-to-diskless switch - #697
Conversation
a9a968f to
8fb0f9e
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes an operational issue where ReplicaManager’s UnderReplicatedPartitions aggregate could remain non-zero (or otherwise not reflect live ISR state) after switching a topic from classic to diskless, by ensuring the leader continues to observe the follower’s fetch state until the replica is back in ISR.
Changes:
- Update follower fetch routing to treat
fetchOffset == classicToDisklessStartOffsetas still eligible for unified-log reads for follower requests, enabling the leader to observe fetch progress at the seal offset. - Ensure diskless followers schedule a fetch even when caught up to the seal but currently out of ISR, so the leader can expand ISR again.
- Add a system test that stops a follower after switch, asserts URP rises, then restarts the follower and asserts URP clears after ISR fully recovers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/kafkatest/tests/inkless/inkless_topic_switch_test.py | Adds URP JMX scraping + a new test to verify URP rises on replica loss and clears after replica recovery post-switch. |
| core/src/main/scala/kafka/server/ReplicaManager.scala | Adjusts diskless follower fetch eligibility at the seal offset and ensures fetching is scheduled when out of ISR even if already caught up. |
| core/src/main/scala/kafka/server/ReplicaFetcherThread.scala | Prevents evicting switched partitions from the replica fetcher until the replica is back in ISR. |
8fb0f9e to
012e6d5
Compare
…to-diskless switch A follower that dropped out of ISR and recovered after a classic-to-diskless switch stayed under-replicated forever, keeping UnderReplicatedPartitions stuck. - Leader fetch handler: record a switched follower's fetch state at the seal so ISR can re-expand, gated on a leader-epoch check (no diskless data read locally). - makeFollower: give a switched, at-seal, out-of-ISR follower a catch-up fetcher. - ReplicaFetcherThread: don't self-evict a switched partition until it's in ISR. Adds unit tests for the epoch-gated seal fetch and a URP-recovery system test.
012e6d5 to
acdcb9e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/kafkatest/tests/inkless/inkless_topic_switch_test.py:409
- _live_cluster_jmx_sum can return 0 even when one or more live brokers failed to report the metric (e.g., JMX read failed or no samples yet). That can make wait-for-zero checks pass prematurely and mask a real UnderReplicatedPartitions > 0 on an unsampled broker. Consider treating partial observations as a scrape miss and returning None unless every live broker produced a value for the requested gauge.
if time_to_stats:
latest = max(time_to_stats.keys())
total += time_to_stats[latest].get(key, 0)
observed = True
return int(total) if observed else None
core/src/test/scala/unit/kafka/server/ReplicaManagerInklessTest.scala:7169
- The comment describes the follower's leader epoch as "STALE (ahead-of-leader)", but an epoch greater than the leader's is not stale; it's mismatched/ahead-of-leader. Tightening the wording makes the test intent clearer.
// Follower fetches at the seal, but carries a STALE (ahead-of-leader) leader epoch. This mirrors
// the classic read path, which validates the request epoch before touching follower state.
core/src/main/scala/kafka/server/ReplicaFetcherThread.scala:174
- The eviction comment says the fetcher stops only once the replica is in ISR, but the condition also allows eviction for consolidating topics even if not in ISR. Updating the comment avoids a mismatch between documentation and behavior.
// Stop fetching after the switch from classic to diskless is completed: once the controller
// has committed a classicToDisklessStartOffset for this partition, our local LEO has reached it,
// and this replica is in ISR, the follower is fully caught up to the leader's frozen classic log
// and must not keep fetching.
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
0da1365 to
88d5b94
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
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/ReplicaManager.scala:2523
- Prechecking with
getReplica(...).foreachsuppresses the classic invalid-replica response: an unassigned follower skipsfetchRecordsand receivesErrors.NONE. CallfetchRecordsdirectly sofollowerReplicaOrThrowreturnsUNKNOWN_LEADER_EPOCH/NOT_LEADER_OR_FOLLOWERas appropriate.
partition.getReplica(params.replicaId).foreach { _ =>
core/src/main/scala/kafka/server/ReplicaManager.scala:2521
- This equality guard bypasses
Partition.fetchRecordsprecisely when the request epoch is invalid. Classic follower reads returnFENCED_LEADER_EPOCHfor an older epoch andUNKNOWN_LEADER_EPOCHfor a newer one, but this path returnsNONE; invoke the classic validation and let the existing catch convert those exceptions to partition errors.
val requestEpochMatchesLeader =
fetchPartitionData.currentLeaderEpoch.toScala.forall(_.intValue() == partition.getLeaderEpoch)
if (requestEpochMatchesLeader) {
Structured review of #697: per-blocker traces with the invariant each one breaks, a decisions index, and the revert-experiment results establishing which of the PR's tests are discriminating. The six review(pr697) commits beneath this one are the proposed fixes, one per finding, each verified by compile plus a targeted test run. They are proposals to cherry-pick, not a branch to merge.
jeqo
left a comment
There was a problem hiding this comment.
Looks good, just a few more comments and a finding:
ReplicationControlManager#expandIsrForDisklessManagedPartitions re-adds an unfenced broker to the ISR of every diskless.enable=true partition without consulting classicToDisklessStartOffset. I wrote that in #643 for born-diskless managed-RF, where any live replica is current because all the data is in object storage but missed the switched case, where the classic prefix below the seal exists only in the replicas' local logs. So today a returning replica can enter a switched partition's ISR with no evidence its prefix is complete, and is then electable.
I can open a follow-up PR to skip partitions whose seal isn't NO_CLASSIC_TO_DISKLESS_START_OFFSET to let them earn ISR through the path this PR builds, if this feels like stretching the scope of this PR.
…nsion (#754) expandIsrForDisklessManagedPartitions re-admitted a returning broker to the ISR of every diskless.enable=true partition, selecting on the topic config alone. That is correct for born-diskless partitions, the case #643 targeted: all their data is in object storage, so any live replica is current. A partition switched from classic breaks that premise. Its records below classicToDisklessStartOffset exist only in the replicas' local logs, so a returning replica could hold an incomplete prefix and was still placed in ISR and made electable. Skip any partition whose seal is not NO_CLASSIC_TO_DISKLESS_START_OFFSET, which covers both a committed seal and a switch still PENDING. Those partitions earn ISR through AlterPartition once the leader observes follower fetch state at the seal -- the path #697 added. Removing this shortcut before that path existed would have left switched partitions with no ISR-recovery route at all. The test drives one unfence across three topics -- born-diskless, switched with a committed seal, and switch-pending. Asserting only that switched partitions stay out would also pass against a guard that skipped everything, so the born-diskless leg is what makes it discriminating. Verified red-then-green: with the guard reverted, the switched assertion fails with expected <false> but was <true>.
…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. A broker restart is enough to trigger this, so a rolling restart reaches every partition of a switched topic: the shutdown drops the replica from ISR, and after the restart it never rejoins. UnderReplicatedPartitions stays non-zero and no later restart recovers it, which is the symptom #697 set out to clear. 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. 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.
…to-diskless switch (#697) A follower that dropped out of ISR and recovered after a classic-to-diskless switch stayed under-replicated forever, keeping UnderReplicatedPartitions stuck. - Leader fetch handler: record a switched follower's fetch state at the seal so ISR can re-expand, gated on a leader-epoch check (no diskless data read locally). - makeFollower: give a switched, at-seal, out-of-ISR follower a catch-up fetcher. - ReplicaFetcherThread: don't self-evict a switched partition until it's in ISR. Adds unit tests for the epoch-gated seal fetch and a URP-recovery system test. --------- Co-authored-by: Viktor Somogyi-Vass <viktorsomogyi@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…nsion (#754) expandIsrForDisklessManagedPartitions re-admitted a returning broker to the ISR of every diskless.enable=true partition, selecting on the topic config alone. That is correct for born-diskless partitions, the case #643 targeted: all their data is in object storage, so any live replica is current. A partition switched from classic breaks that premise. Its records below classicToDisklessStartOffset exist only in the replicas' local logs, so a returning replica could hold an incomplete prefix and was still placed in ISR and made electable. Skip any partition whose seal is not NO_CLASSIC_TO_DISKLESS_START_OFFSET, which covers both a committed seal and a switch still PENDING. Those partitions earn ISR through AlterPartition once the leader observes follower fetch state at the seal -- the path #697 added. Removing this shortcut before that path existed would have left switched partitions with no ISR-recovery route at all. The test drives one unfence across three topics -- born-diskless, switched with a committed seal, and switch-pending. Asserting only that switched partitions stay out would also pass against a guard that skipped everything, so the born-diskless leg is what makes it discriminating. Verified red-then-green: with the guard reverted, the switched assertion fails with expected <false> but was <true>.
…to-diskless switch (#697) A follower that dropped out of ISR and recovered after a classic-to-diskless switch stayed under-replicated forever, keeping UnderReplicatedPartitions stuck. - Leader fetch handler: record a switched follower's fetch state at the seal so ISR can re-expand, gated on a leader-epoch check (no diskless data read locally). - makeFollower: give a switched, at-seal, out-of-ISR follower a catch-up fetcher. - ReplicaFetcherThread: don't self-evict a switched partition until it's in ISR. Adds unit tests for the epoch-gated seal fetch and a URP-recovery system test. --------- Co-authored-by: Viktor Somogyi-Vass <viktorsomogyi@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…nsion (#754) expandIsrForDisklessManagedPartitions re-admitted a returning broker to the ISR of every diskless.enable=true partition, selecting on the topic config alone. That is correct for born-diskless partitions, the case #643 targeted: all their data is in object storage, so any live replica is current. A partition switched from classic breaks that premise. Its records below classicToDisklessStartOffset exist only in the replicas' local logs, so a returning replica could hold an incomplete prefix and was still placed in ISR and made electable. Skip any partition whose seal is not NO_CLASSIC_TO_DISKLESS_START_OFFSET, which covers both a committed seal and a switch still PENDING. Those partitions earn ISR through AlterPartition once the leader observes follower fetch state at the seal -- the path #697 added. Removing this shortcut before that path existed would have left switched partitions with no ISR-recovery route at all. The test drives one unfence across three topics -- born-diskless, switched with a committed seal, and switch-pending. Asserting only that switched partitions stay out would also pass against a guard that skipped everything, so the born-diskless leg is what makes it discriminating. Verified red-then-green: with the guard reverted, the switched assertion fails with expected <false> but was <true>.
A follower that dropped out of ISR and recovered after a classic-to-diskless switch stayed under-replicated forever, keeping
UnderReplicatedPartitionsstuck.