[PR] [fix][test] Fix flaky TestReplicationWorker.testRepairedNotAdheringPlacementPolicyLedgerFragmentsOnRack [pulsar]

["lhotari (via GitHub)" <gi...@apache.org>]

lhotari opened a new pull request, #26308:
URL: https://github.com/apache/pulsar/pull/26308

   Fixes #26307
   
   ### Motivation
   
   `TestReplicationWorker.testRepairedNotAdheringPlacementPolicyLedgerFragmentsOnRack` failed both TestNG
   attempts in [this `CI - Unit - Pulsar Metadata` run](https://github.com/apache/pulsar/actions/runs/31364318604/job/93381096376),
   on `assertNotNull(stat)`. The shared helper `testRepairedNotAdheringPlacementPolicyLedgerFragments(...)`
   that backs it (and `testReplicationStats`) has two independent timing problems.
   
   **1. The underreplication mark is read before it is written.**
   
   `AuditorPlacementPolicyCheckTask.doPlacementPolicyCheck()` fires
   `ledgerUnderreplicationManager.markLedgerUnderreplicatedAsync(...)` and immediately calls
   `iterCallback.processResult(OK, ...)` without awaiting it. So the `placementPolicyCheck` latch, the
   `NUM_LEDGERS_NOT_ADHERING_TO_PLACEMENT_POLICY` gauge and the `PLACEMENT_POLICY_CHECK_TIME` success
   count — the only signals the test waits on — are all recorded while the ZooKeeper write creating
   `/ledgers/underreplication/ledgers/0000/0000/0000/0000/urL0000000000` may still be in flight. The test
   then read that znode with a bare `zk.exists(...)`.
   
   In the failing run the mark was issued at `07:16:30,762` and the assertion ran ~490ms later, on an
   attempt where the one-ledger check alone reported `durationMs=409` amid ZK session churn. The test
   report contains no delete of that znode and no "mark to under replication manager failed" error, so
   the write had simply not landed yet.
   
   `AuditorPlacementPolicyCheckTest.testPlacementPolicyCheckWithLedgersNotAdheringToPlacementPolicyAndMarkToUnderreplication`
   has the identical defect — a one-shot `pollLedgerToRereplicate()` after `auditor.close()` — so it is
   fixed here too rather than left as a known twin.
   
   **2. The later awaits were running on a few hundred milliseconds of slack.**
   
   In the `OnRack` variant the `ReplicationWorker` is started *before* the `/rack2` bookie exists, so its
   first 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 are
   spent inside the `Awaitility.await()` calls that follow, which use the default 10s timeout. Every local
   invocation took ~11.1s and logged `deferring the ledger lock release {delayMs=9375}`. A second failed
   attempt 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 the `ZooKeeper` handle above the `try`, and wait for
     the underreplication mark with `Awaitility` *inside* it, while the auditor is still running — so the
     wait 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's `ReplicationWorker` from a dedicated `ServerConfiguration`
     with `lockReleaseOfFailedLedgerGracePeriod=64` and `rwRereplicateBackoffMs=100`, the way the other
     `ReplicationWorker` tests in this class already do, so the awaits are not competing with ~14s of fixed
     delay. This also stops the helper mutating (and having to restore) the shared `baseConf`.
   - `AuditorPlacementPolicyCheckTest`: move the `pollLedgerToRereplicate()` assertion into the `try` block
     and poll it with `Awaitility`, mirroring the fix above. The assertion's argument order is corrected to
     `AssertJUnit`'s `(expected, actual)`.
   
   No assertion was weakened or removed: the same znode/ledger id, the same expectations.
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   This change is already covered by existing tests, such as
   `TestReplicationWorker.testRepairedNotAdheringPlacementPolicyLedgerFragmentsOnRack`,
   `TestReplicationWorker.testReplicationStats` and
   `AuditorPlacementPolicyCheckTest.testPlacementPolicyCheckWithLedgersNotAdheringToPlacementPolicyAndMarkToUnderreplication`.
   
   Verified locally with a temporary `invocationCount` (removed before this PR): 10/10, 5/5 and 10/10
   passes respectively. The deferred lock release drops from `delayMs=9375` to `delayMs=2`, and the
   flaking 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 (`TestReplicationWorker` 14/14, `AuditorPlacementPolicyCheckTest`
   8/8), and `./gradlew quickCheck` is clean.
   
   ### Does this pull request potentially affect one of the following parts:
   
   *If the box was checked, please highlight the changes*
   
   - [ ] 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
   
   ### Documentation
   
   - [ ] `doc-required`
   - [x] `doc-not-needed`
   - [ ] `doc`
   - [ ] `doc-complete`
   
   Test-only change.
   


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [fix][test] Fix flaky TestReplicationWorker.testRepairedNotAdheringPlacementPolicyLedgerFragmentsOnRack [pulsar]

["nodece (via GitHub)" <gi...@apache.org>]

nodece merged PR #26308:
URL: https://github.com/apache/pulsar/pull/26308


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org