lhotari opened a new pull request, #26106:
URL: https://github.com/apache/pulsar/pull/26106
### Motivation
When a publish to the remote cluster fails, `PersistentReplicator` rewinds
the cursor
(`beforeTerminateOrCursorRewinding(Failed_Publishing)` + `doRewindCursor`)
so the unacked messages
are re-read. If a cursor read was already dispatched at that moment,
`cursor.cancelPendingReadRequest()`
returns `false` (there is no waiting read op to cancel — the common case
when there is a backlog), so
`cancelPendingReadTasks()` only flags the in-flight task and does not
complete it.
When that dispatched read later completes, `readEntriesComplete()` discarded
the result in the skip
branch but left the task with `entries == null`. This caused a two-fold
stall:
1. The task stayed `entries == null` forever, so `hasPendingRead()` stayed
`true` and
`getPermitsIfNoPendingRead()` returned `0` — no further reads were ever
issued.
2. `ManagedCursor` advances the read position when a read completes
(`OpReadEntry` → `ManagedCursorImpl.setReadPosition`), so the discarded
read moved the read position
past the still-unacked messages, overwriting the rewind from
`doRewindCursor()`. Even once reads
resumed, those messages were stranded behind the read position and never
re-read.
The geo-replication backlog therefore never drained. This is independent of
the replicator dispatch
rate-limiter path; it exists on `master` regardless of #26005 (it was found
while reviewing that PR).
### Modifications
In the `readEntriesComplete()` skip branch, when the read result is
discarded due to a cursor rewind
and the replicator is not terminating:
- Rewind the cursor again so the messages stranded by the completing read
are re-read, and complete the
task with an empty result so it releases its permit and stops counting as
a pending read. Both are
done under `synchronized(inFlightTasks)` — the same lock
`doRewindCursor()` uses — so they are atomic
with respect to `readMoreEntries()`.
- Resume reading on the broker executor instead of calling
`readMoreEntries()` directly, to avoid deep
`readEntriesComplete -> readMoreEntries` recursion when a cursor read
completes inline
(`StackOverflowError`), the same hazard
`PersistentDispatcherMultipleConsumers.readMoreEntriesAsync()`
guards against.
The previous `incCompletedEntries()` call in the skip branch was a no-op
there (the task's `entries`
field is always `null` at that point) and is replaced by completing the task
with an empty result.
### 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.testCursorRewindSkippedReadCompletesInFlightTask"`
— a deterministic unit test that a skip-flagged in-flight read completes
the task and frees the read slot.
- `./gradlew :pulsar-broker:test --tests
"org.apache.pulsar.broker.service.persistent.PersistentReplicatorInflightTaskTest.testReplicationRecoversAfterPublishFailureRewindWithInflightRead"`
— an end-to-end test over two clusters that holds a real cursor read in
flight, rewinds the cursor the
same way a failed publish does, then asserts the backlog drains and every
message is delivered to the
remote cluster.
Both tests fail on `master` (the e2e test times out with the backlog stuck)
and pass with the fix. Both
were run with `invocationCount = 5` with no flakiness.
### Does this pull request potentially affect one of the following parts:
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
