[fix][meta] Preserve ownership during underreplication lock cleanup - #26794
Denovo1998 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
lhotari
left a comment
There was a problem hiding this comment.
Thanks for working through the ownership edge cases in underreplication lock cleanup; the per-acquisition id in the text-format comment is a neat way to stay readable for BookKeeper, and the tests run against every backend.
The approach holds up well. I reverted PulsarLedgerUnderreplicationManager.java to the base in a scratch checkout and 27 of the 165 tests in LedgerUnderreplicationManagerTest fail, so the new tests do pin the fix. Two gaps remain, inline: the conditional delete does not protect against a node being recreated between the ownership read and the delete on backends whose versions restart at 0, and cleanup while a migration is in the FAILED phase can still leave the target-store copy behind. The first one is narrow, so the code comment/PR description may be enough if a stronger fence is not available; the second is probably best handled in DualMetadataStore or documented as a follow-up.
Sorry, something went wrong.
| } | ||
| return CompletableFuture.<Void>completedFuture(null); | ||
| } | ||
| return store.delete(lock.getLockPath(), Optional.of(existing.get().getStat().getVersion())); |
There was a problem hiding this comment.
[SUGGESTION] the version-conditional delete does not guard against a replacement lock on ZooKeeper, Memory and RocksDB
The lock node is never updated after creation, so its version is always 0 on ZooKeeper (and Memory/RocksDB restart the version when a path is recreated). If our session expires after the store.get above but before this delete runs, and another worker then acquires the same lock path, the replacement also has version 0 and delete(path, Optional.of(0)) removes it. The lock-id and isCreatedBySelf checks are done on the earlier read, not atomically with the delete. Oxia is fine because its version ids are unique per modification.
The window is small and this is much tighter than the unconditional delete it replaces, so I would not block on it. Two options: say in the comment that the conditional delete only narrows the race on version-resetting backends, or, if a backend-level fence is practical, tie the delete to the acquisition incarnation. The existing replacement tests (testLockCleanupPreservesReplacementInSameSession and friends) replace the node before the ownership read, so a deterministic test that replaces it between the read and the delete would show what is and is not covered.
Sorry, something went wrong.
| new MetadataStoreException("Metadata migration changed during lock cleanup; retry")); | ||
| } | ||
| if (existing.isEmpty() || !ownsLock(ledgerId, lock, existing.get())) { | ||
| if (migrationPhase == MigrationPhase.FAILED) { |
There was a problem hiding this comment.
[SUGGESTION] cleanup in the FAILED migration phase can leave the target-store copy of an owned lock behind
The FAILED guard only applies when the source lock is missing or not ours. If the source copy is still ours (migration failed after PREPARATION already recreated the ephemeral node in the target), we skip the guard, delete the source copy and drop the entry from heldLocks. DualMetadataStore.delete in FAILED also removes the path from localEphemeralPaths and only touches the source store, so a later migration retry does not delete the target copy, and the target session keeps it alive, blocking rereplication for that ledger after cutover.
This is a general limitation of DualMetadataStore ephemerals rather than something this PR introduced, so a follow-up is fine, but since the comment above claims the failed-migration case is handled, it would be worth either deleting the owned copy from the target too (for example via a DualMetadataStore hook) or narrowing the comment. A case in testLockCleanupAfterMetadataMigration that runs cleanup while the phase is FAILED and the source lock is still present would pin it.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Motivation
Underreplication locks can survive session-loss notifications and metadata-store migration. Losing their local ownership records or treating failed cleanup as successful can leave orphaned locks that block ledger rereplication. Cleanup must retain ownership until the acquisition is safely released and preserve locks belonging to another acquisition.
Modifications
Verifying this change
This change added tests and can be verified as follows:
TEST_METADATA_PROVIDERS=Memory,ZooKeeper,MockZooKeeper,RocksDB,Oxia \ ./gradlew :pulsar-metadata:test \ --tests 'org.apache.pulsar.metadata.bookkeeper.LedgerUnderreplicationManagerTest' \ -PtestRetryCount=0 -PtestFailFast=false --max-workers=4 ./gradlew quickCheckLocal validation passed all 165 tests with no failures, errors, or skipped tests. quickCheck also passed. The new regressions reproduced the cleanup failures before the corresponding fixes.
Does this pull request potentially affect one of the following parts: