[fix][test] Fix flaky TestReplicationWorker.testRepairedNotAdheringPlacementPolicyLedgerFragmentsOnRack - #26308
Open
lhotari wants to merge 1 commit into
Conversation
…acementPolicyLedgerFragments* The auditor's placementPolicyCheck marks a non-adhering ledger underreplicated with a fire-and-forget markLedgerUnderreplicatedAsync write that it does not await before recording its stats. Tests that wait only on those stats and then read the underreplication znode can therefore read it too early. Wait for the mark with Awaitility, while the auditor is still running, in both TestReplicationWorker and its twin in AuditorPlacementPolicyCheckTest. Also build the test's ReplicationWorker from a dedicated ServerConfiguration with a short lockReleaseOfFailedLedgerGracePeriod and rwRereplicateBackoffMs. The worker's first replication attempt necessarily fails because the /rack2 bookie is started afterwards, and the resulting 9375ms deferred lock release plus the 5000ms run-loop backoff consumed nearly all of the following awaits' default 10s timeout. This also stops the helper mutating the shared baseConf. Assisted-by: Claude Code (Opus 5)
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.
Fixes #26307
Motivation
TestReplicationWorker.testRepairedNotAdheringPlacementPolicyLedgerFragmentsOnRackfailed both TestNGattempts in this
CI - Unit - Pulsar Metadatarun,on
assertNotNull(stat). The shared helpertestRepairedNotAdheringPlacementPolicyLedgerFragments(...)that backs it (and
testReplicationStats) has two independent timing problems.1. The underreplication mark is read before it is written.
AuditorPlacementPolicyCheckTask.doPlacementPolicyCheck()firesledgerUnderreplicationManager.markLedgerUnderreplicatedAsync(...)and immediately callsiterCallback.processResult(OK, ...)without awaiting it. So theplacementPolicyChecklatch, theNUM_LEDGERS_NOT_ADHERING_TO_PLACEMENT_POLICYgauge and thePLACEMENT_POLICY_CHECK_TIMEsuccesscount — the only signals the test waits on — are all recorded while the ZooKeeper write creating
/ledgers/underreplication/ledgers/0000/0000/0000/0000/urL0000000000may still be in flight. The testthen read that znode with a bare
zk.exists(...).In the failing run the mark was issued at
07:16:30,762and the assertion ran ~490ms later, on anattempt where the one-ledger check alone reported
durationMs=409amid ZK session churn. The testreport contains no delete of that znode and no "mark to under replication manager failed" error, so
the write had simply not landed yet.
AuditorPlacementPolicyCheckTest.testPlacementPolicyCheckWithLedgersNotAdheringToPlacementPolicyAndMarkToUnderreplicationhas the identical defect — a one-shot
pollLedgerToRereplicate()afterauditor.close()— so it isfixed here too rather than left as a known twin.
2. The later awaits were running on a few hundred milliseconds of slack.
In the
OnRackvariant theReplicationWorkeris started before the/rack2bookie exists, so itsfirst replication attempt necessarily fails and defers the ledger lock release by
lockReleaseOfFailedLedgerGracePeriod / 2^5= 9375ms (BookKeeper's default grace period is 300000),after which the run loop backs off for another
rwRereplicateBackoffMs= 5000ms. Those delays arespent inside the
Awaitility.await()calls that follow, which use the default 10s timeout. Every localinvocation took ~11.1s and logged
deferring the ledger lock release {delayMs=9375}. A second failedattempt would defer 18750ms, which the 10s await cannot survive at all.
The fire-and-forget write is upstream BookKeeper behaviour, present on apache/bookkeeper master as
well; this PR only fixes the tests.
Modifications
TestReplicationWorker: hoist the znode path and theZooKeeperhandle above thetry, and wait forthe underreplication mark with
Awaitilityinside it, while the auditor is still running — so thewait does not race auditor shutdown and the 1s periodic placement policy check can retry a failed
write. The second use of the path literal now reuses the local.
TestReplicationWorker: build the test'sReplicationWorkerfrom a dedicatedServerConfigurationwith
lockReleaseOfFailedLedgerGracePeriod=64andrwRereplicateBackoffMs=100, the way the otherReplicationWorkertests in this class already do, so the awaits are not competing with ~14s of fixeddelay. This also stops the helper mutating (and having to restore) the shared
baseConf.AuditorPlacementPolicyCheckTest: move thepollLedgerToRereplicate()assertion into thetryblockand poll it with
Awaitility, mirroring the fix above. The assertion's argument order is corrected toAssertJUnit's(expected, actual).No assertion was weakened or removed: the same znode/ledger id, the same expectations.
Verifying this change
This change is already covered by existing tests, such as
TestReplicationWorker.testRepairedNotAdheringPlacementPolicyLedgerFragmentsOnRack,TestReplicationWorker.testReplicationStatsandAuditorPlacementPolicyCheckTest.testPlacementPolicyCheckWithLedgersNotAdheringToPlacementPolicyAndMarkToUnderreplication.Verified locally with a temporary
invocationCount(removed before this PR): 10/10, 5/5 and 10/10passes respectively. The deferred lock release drops from
delayMs=9375todelayMs=2, and theflaking test from ~11.1s to ~1.9s per invocation, so the awaits now have ~9.8s of headroom instead of
~0.5s. Both full test classes pass (
TestReplicationWorker14/14,AuditorPlacementPolicyCheckTest8/8), and
./gradlew quickCheckis clean.Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc-requireddoc-not-neededdocdoc-completeTest-only change.