Skip to content

[fix][test] Fix OneWayReplicatorTest.testProbBKErrorWhenReplicating timeout - #26581

Open
hanmz wants to merge 1 commit into
apache:masterfrom
hanmz:fix/testProbBKErrorWhenReplicating-timeout
Open

hanmz wants to merge 1 commit into
apache:masterfrom
hanmz:fix/testProbBKErrorWhenReplicating-timeout

Conversation

@hanmz

@hanmz hanmz commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Motivation

OneWayReplicatorTest.testProbBKErrorWhenReplicating declares a 45 second method timeout:

@Test(timeOut = 45 * 1000)
public void testProbBKErrorWhenReplicating() throws Exception {

but internally waits up to 600 seconds for the replication backlog to drain:

Awaitility.await().atMost(Duration.ofSeconds(600)).pollInterval(Duration.ofSeconds(1)).untilAsserted(() -> {
    TopicStats topicStats = admin1.topics().getStats(topicName);
    assertEquals(topicStats.getReplication().get(cluster2).getReplicationBacklog(), 0);
});

The test publishes 10,000 messages and injects ManagedLedgerException.TooManyRequestsException with 50% probability on every read. Draining the replication backlog under that error injection takes far longer than 45 seconds (around 150 seconds when I ran it locally), so the TestNG timeout always fires first and the 600 second Awaitility budget is unreachable by construction.

The result is a ThreadTimeoutException: Method didn't finish within the time-out 45000 rather than a meaningful assertion failure.

This 45 second limit has already caused trouble before: #26123 changed makeReadEntryProbFail to accept a caller-provided executor specifically because routing reads through ForkJoinPool.commonPool() starved this test and made it exceed its 45 second limit on CI. That change removed one source of slowdown, but the underlying mismatch between the declared timeout and the work the test performs was left in place.

Modifications

Raise the method timeout from 45 seconds to 300 seconds so it is consistent with the amount of work the test actually performs, while still keeping a bound on the run time.

-    @Test(timeOut = 45 * 1000)
+    @Test(timeOut = 300 * 1000)
     public void testProbBKErrorWhenReplicating() throws Exception {

300 seconds gives roughly a 2x margin over the observed ~150 second runtime, which should absorb slower CI machines without letting a genuinely hung test run indefinitely. Other tests in this class already use timeouts up to 240 seconds, so this is in line with existing practice in the file.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

The modified test was run locally before and after the change:

  • Before: fails consistently with ThreadTimeoutException at 45s (actual runtime ~150s)
  • After: passes consistently (~150s, well within the 300s budget)

Does this pull request potentially affect one of the following parts:

  • Dependencies (add or upgrade a dependency): no
  • The public API: no
  • The schema: no
  • The default values of configurations: no
  • The threading model: no
  • The binary protocol: no
  • The REST endpoints: no
  • The admin CLI options: no
  • The metrics: no
  • Anything that affects deployment: no

Documentation

  • doc-not-needed

Matching PR in forked repository

PR in forked repository: https://github.com/hanmz/pulsar/pull/new/fix/testProbBKErrorWhenReplicating-timeout

…imeout

The test declares @test(timeOut = 45 * 1000) but internally waits up to
600 seconds via Awaitility for the replication backlog to drain:

    Awaitility.await().atMost(Duration.ofSeconds(600))

With 10,000 messages and a 50% probability of injected
ManagedLedgerException.TooManyRequestsException, replication takes far
longer than 45 seconds (around 150 seconds locally), so the TestNG
timeout always fires first and the Awaitility budget is never reachable.

Raise the method timeout to 300 seconds so it is consistent with the
work the test actually performs, while still keeping a bound on the run
time. Other tests in this class already use timeouts up to 240 seconds.
}

@Test(timeOut = 45 * 1000)
@Test(timeOut = 300 * 1000)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we reduce totalMsg instead? CI tests are already too slow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants