lhotari commented on PR #26106:
URL: https://github.com/apache/pulsar/pull/26106#issuecomment-4848761650
### Why #26005 makes this stall much more likely (and why
`testReplicationWithSchema` now fails almost every run)
The stall requires a cursor read to be **in flight** (`entries == null`,
flagged-but-not-cancellable) at the moment the schema-fetch rewind runs
synchronously inside `GeoPersistentReplicator.replicateEntries`
(`beforeTerminateOrCursorRewinding(Fetching_Schema)`). How often that
coincidence happens is governed by how the replicator sizes and paces its reads
— which #26005 changed.
**Before #26005**, the default read path (no dispatch rate limiter — the
case in this test) read `getPermitsIfNoPendingRead()` entries, i.e. up to
`replicationProducerQueueSize` (default **1000**), with `byteSize = -1`
(unbounded), and did **not** apply `readBatchSize`:
```java
// pre-#26005 readMoreEntries(), rate-limiter-disabled branch:
cursor.asyncReadEntriesOrWait(newInFlightTask.readingEntries /* up to
producerQueueSize */, -1, ...);
```
So a replicator draining this test's 1000-message backlog requested it in
**essentially one read**. The `PersonOne → PersonThree` schema boundary (msg
~500) fell inside that single batch, leaving little opportunity for a
*separate* read to be in flight when the rewind fired — consistent with the
stall being latent/rare before #26005.
**After #26005** ("honor readBatchSize/maxReadSizeBytes on the default read
path"), every read now goes through `getReadLimits(permits)`, which caps it to
`min(permits, readBatchSize)` messages and `readMaxSizeBytes`. By default
`readBatchSize = min(replicationProducerQueueSize, dispatcherMaxReadBatchSize)
=` **`100`**.
(Worth noting for the record: `readBatchSize` does **not** start at 1 in
steady state — it is initialized to `getMaxReadBatchSize()` = 100 in the
constructor. It only drops to `dispatcherMinReadBatchSize = 1` and ramps back
up by doubling after a *read failure*, in `readEntriesFailed` ("reduce read
batch size to avoid flooding bookies with retries"). So on the normal
backlog-drain path it is a constant 100.)
So the same backlog is now drained in **~10 reads of 100**, continuously
pipelined — each send-completion frees producer-queue permits and re-triggers
`readMoreEntries` while the current batch is still replicating. With ~10× more,
smaller, pipelined reads, a cursor read is in flight far more often when
`replicateEntries` reaches the schema boundary, and that in-flight read is
exactly what the `Fetching_Schema` rewind strands.
This matches CI: on [run
28466180778](https://github.com/apache/pulsar/actions/runs/28466180778/attempts/3?pr=26122)
(a branch based on `master` without this fix), `testReplicationWithSchema`
failed in **all three** re-run attempts — i.e. it is now near-deterministic
rather than an occasional flake.
So #26005 did not introduce this bug, but by bounding the default read path
to `readBatchSize` it turned a latent/rare stall into a frequent one. This fix
is effectively resolving that frequency regression, which argues for merging it
promptly.
--
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]
