[fix][broker] Fix replicator stall when a skipped pending read completes after a cursor rewind - #26112
Closed
lhotari wants to merge 1 commit into
Closed
[fix][broker] Fix replicator stall when a skipped pending read completes after a cursor rewind#26112lhotari wants to merge 1 commit into
lhotari wants to merge 1 commit into
Conversation
…tes after a cursor rewind ### Motivation `PersistentReplicator.readEntriesComplete()` has a "skip" branch that runs when the replicator has terminated or when the read result must be discarded because the cursor was rewound (`InFlightTask.isSkipReadResultDueToCursorRewind()`). A cursor rewind happens, for example, when a new schema is detected and the replicator pauses to fetch it. When a cursor rewind cancels in-flight reads, `cancelPendingReadTasks()` only resets the pending reading task to a completed state when `cursor.cancelPendingReadRequest()` returns `true`. If the read has already been dispatched to BookKeeper it cannot be cancelled, so the request returns `false` and the pending task is left with `entries == null` while still flagged with `skipReadResultDueToCursorRewind == true`. That dispatched read still delivers a `readEntriesComplete()` callback. The skip branch then calls `InFlightTask.incCompletedEntries()` for each returned entry, but because the task's `entries` field is still `null`, `incCompletedEntries()` cannot increment `completedEntries` and instead logs "Unexpected calling of increase completed entries". The task therefore never becomes `isDone()`: it permanently occupies the single read slot (`getPermitsIfNoPendingRead()` returns 0 and `hasPendingRead()` returns true), so `readMoreEntries()` can never schedule another read and replication for that cluster stalls. This was observed as a stall at a schema boundary, e.g. flaky failures of `ReplicatorTest.testReplicationWithSchema` where messages produced with a second schema were never replicated and consumers on the remote clusters timed out. ### Modifications - In `readEntriesComplete()`'s skip branch, record the read result on the in-flight task (`setEntries(entries)`) before completing it, so the task becomes `isDone()` and releases its read permit, allowing the read loop to resume. - Add `PersistentReplicatorInflightTaskTest.testSkippedPendingReadDoesNotStallReplication`, a deterministic regression test that reproduces the stall (a pending read flagged skip whose dispatched read then completes) and asserts the task is reconciled and read permits restored. Assisted-by: Claude Code (Claude Opus 4.8) Claude-Session: https://claude.ai/code/session_01CMKzAniMcNHvk1jBh6bEKQ
Member
Author
|
Closing as a duplicate of #26106, which fixes the same bug (a cursor-rewind-skipped in-flight read leaving the This PR only completed the task to release its read permit. #26106 additionally:
both under the |
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.
Motivation
PersistentReplicator.readEntriesComplete()has a "skip" branch that runs when the replicator has terminated or when the read result must be discarded because the cursor was rewound (InFlightTask.isSkipReadResultDueToCursorRewind()). A cursor rewind happens, for example, when a new schema is detected and the replicator pauses to fetch it.When a cursor rewind cancels in-flight reads,
cancelPendingReadTasks()only resets the pending reading task to a completed state whencursor.cancelPendingReadRequest()returnstrue. If the read has already been dispatched to BookKeeper it cannot be cancelled, so the request returnsfalseand the pending task is left withentries == nullwhile still flagged withskipReadResultDueToCursorRewind == true.That dispatched read still delivers a
readEntriesComplete()callback. The skip branch then callsInFlightTask.incCompletedEntries()for each returned entry, but because the task'sentriesfield is stillnull,incCompletedEntries()cannot incrementcompletedEntriesand instead logsUnexpected calling of increase completed entries. The task therefore never becomesisDone(): it permanently occupies the single read slot (getPermitsIfNoPendingRead()returns0andhasPendingRead()returnstrue), soreadMoreEntries()can never schedule another read and replication for that direction stalls.This was observed as a stall at a schema boundary — e.g. flaky failures of
ReplicatorTest.testReplicationWithSchema, where messages produced with a second schema were never replicated and the consumers on the remote clusters timed out. The race requires a cursor read to already be dispatched to BookKeeper (so it can no longer be cancelled) at the moment the schema-fetch rewind runs, which is why it reproduces under the slower I/O timing of CI but is hard to reproduce on a fast local machine.Modifications
readEntriesComplete()'s skip branch, record the read result on the in-flight task (setEntries(entries)) before completing it, so the task becomesisDone()and releases its read permit, allowing the read loop to resume.PersistentReplicatorInflightTaskTest.testSkippedPendingReadDoesNotStallReplication, a deterministic regression test that reproduces the stall (a pending read flagged skip whose dispatched read then completes) and asserts the task is reconciled and read permits are restored.Verifying this change
This change added tests and can be verified as follows:
./gradlew :pulsar-broker:test --tests "org.apache.pulsar.broker.service.persistent.PersistentReplicatorInflightTaskTest.testSkippedPendingReadDoesNotStallReplication"— the new test fails onmaster(the in-flight task is never completed, so read permits are never restored) and passes with this change../gradlew :pulsar-broker:test --tests "org.apache.pulsar.broker.service.persistent.PersistentReplicatorInflightTaskTest"and--tests "org.apache.pulsar.broker.service.ReplicatorTest.testReplicationWithSchema"pass.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes