Skip to content

[fix][test] Run makeReadEntryProbFail's errorOrNot on a caller-provided executor - #26123

Merged
merlimat merged 2 commits into
apache:masterfrom
lhotari:lh-fix-ManagedLedgerTest-makeReadEntryProbFail
Jul 1, 2026
Merged

[fix][test] Run makeReadEntryProbFail's errorOrNot on a caller-provided executor#26123
merlimat merged 2 commits into
apache:masterfrom
lhotari:lh-fix-ManagedLedgerTest-makeReadEntryProbFail

Conversation

@lhotari

@lhotari lhotari commented Jul 1, 2026

Copy link
Copy Markdown
Member

Motivation

ManagedLedgerTest.makeReadEntryProbFail installs a Mockito answer on
LedgerHandle.readUnconfirmedAsync that consults a caller-supplied
Supplier<ManagedLedgerException> (errorOrNot) to decide whether a read should
fail. errorOrNot was evaluated directly on the thread that calls
readUnconfirmedAsync.

When a test supplies a blocking errorOrNot, that read thread is held for the
whole duration of the block, which can stall or deadlock the test. For example,
PersistentReplicatorInflightTaskTest#testReadEntriesFailedCompletesInFlightTaskAfterReplicatorTerminated
(added in #25767) blocks in errorOrNot on a CountDownLatch until the test
releases it after terminating the replicator. Because the calling thread was
blocked, the test flow could not make progress and its
failRead.await(30, SECONDS) always timed out.

Modifications

  • makeReadEntryProbFail now takes an Executor that runs errorOrNot, so each
    caller decides where it runs (composing the resulting read via thenCompose).
  • OneWayReplicatorTest.testProbBKErrorWhenReplicating passes
    MoreExecutors.directExecutor() to keep read interception inline and ordered
    on the calling thread — its errorOrNot never blocks.
  • The three blocking PersistentReplicatorInflightTaskTest cases pass a
    dedicated single-threaded executor (@Cleanup("shutdownNow")) so a blocking
    errorOrNot runs off the read thread while reads stay ordered.

This only touches test code; production code is unaffected.

Verifying this change

This change is covered by the existing tests that use makeReadEntryProbFail.

Verified locally:

./gradlew :pulsar-broker:test \
  --tests "org.apache.pulsar.broker.service.OneWayReplicatorTest.testProbBKErrorWhenReplicating" \
  --tests "org.apache.pulsar.broker.service.persistent.PersistentReplicatorInflightTaskTest"
  • OneWayReplicatorTest.testProbBKErrorWhenReplicating: passes in ~17s.

  • PersistentReplicatorInflightTaskTest: 15 tests, 0 failures, including
    testReadEntriesFailedCompletesInFlightTaskAfterReplicatorTerminated.

  • Make sure that the change passes the CI checks.

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

…eReadEntryProbFail

The injected readUnconfirmedAsync answer evaluated the errorOrNot supplier on
the thread that calls readUnconfirmedAsync. When a test supplies a blocking
implementation, that read thread is held for the whole duration of the block
and the test can deadlock. For example,
PersistentReplicatorInflightTaskTest#testReadEntriesFailedCompletesInFlightTaskAfterReplicatorTerminated
waits on a CountDownLatch before returning the failure, so its
failRead.await(30, SECONDS) always timed out.

Evaluate errorOrNot with CompletableFuture.supplyAsync so it runs on
ForkJoinPool.commonPool(), leaving the caller thread free, and compose the
resulting read future with thenCompose.
…ed executor

The previous commit routed every intercepted read through
CompletableFuture.supplyAsync (ForkJoinPool.commonPool()) to keep a blocking
errorOrNot off the caller thread. That funnels all managed ledger reads through a
shared, unordered pool and starved
OneWayReplicatorTest.testProbBKErrorWhenReplicating (10k messages), which timed
out its 45s limit on CI where the common pool has few threads.

makeReadEntryProbFail now takes an Executor that runs errorOrNot, letting each
caller decide where it runs:

- OneWayReplicatorTest.testProbBKErrorWhenReplicating passes
  MoreExecutors.directExecutor() to keep read interception inline and ordered on
  the calling thread (its errorOrNot never blocks).
- The blocking PersistentReplicatorInflightTaskTest cases pass a dedicated
  single-threaded executor (@cleanup("shutdownNow")) so a blocking errorOrNot
  runs off the read thread while reads stay ordered.
@lhotari lhotari changed the title [fix][test] Avoid blocking the caller thread in ManagedLedgerTest.makeReadEntryProbFail [fix][test] Run makeReadEntryProbFail's errorOrNot on a caller-provided executor Jul 1, 2026
@merlimat
merlimat merged commit 12d8aa9 into apache:master Jul 1, 2026
44 checks passed
@lhotari lhotari added this to the 5.0.0-M2 milestone Jul 2, 2026
lhotari added a commit that referenced this pull request Jul 2, 2026
lhotari added a commit that referenced this pull request Jul 2, 2026
sandeep-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants