voonhous commented on PR #19486: URL: https://github.com/apache/hudi/pull/19486#issuecomment-5372950177
Pushed three commits addressing review rounds 2 through 5 (the later rounds reviewed the fixes themselves, on two different models); the PR description has been updated to match the final diff, including a squash message for the merging committer. `771e03fa09a9`: - **The exclusive-mode create is now pinned.** `storage.create(path, false)` in `acquireLock` is the provider's cross-process mutual exclusion, and no test anywhere exercised its already-exists arm; flipping the `false` to `true` left everything green. Given `5faefcd01fa8` broke the guarded create for ~2.5 years unnoticed, that gap mattered. `acquireLock` is package-private `@VisibleForTesting`; the loser now fails on `FileAlreadyExistsException` with the winner's payload intact, and `tryLock`'s catch arm (the clean `false` a losing writer sees) is reached deterministically by pointing the lock directory at a regular file. The javadocs say "exclusive-mode create", not "atomic": HDFS guarantees atomicity there, local FS's create is check-then-act. - **The reload fix closed one race window and left the mirror one open.** Checking `exists()` before `open()` handles an already-gone lock file, but a file deleted between the two calls still threw, reproducing the stale-owner symptom this PR fixes. The `exists()` check is gone; `FileNotFoundException` clears the field, pinned via a `hoodie.storage.class` stub whose `exists()` lies while `open()` throws, and `HoodieStorage.open` documents the contract. - **The class could never actually serialize.** `lockInfo` is built eagerly and `LockInfo` is not `Serializable` (since HUDI-5377), and the marker is load-bearing: `CleanActionExecutor`'s Spark closure captures the transaction manager and with it the provider, the HUDI-7782 bug class. `lockInfo`/`sdf` are transient now, `serialVersionUID` pinned at 1L (compat-safe: 0.12.x streams carried a computed UID no 0.13.0+ build could read anyway), round trip covered by test. `428a7197aee0` + `831aed976cbf` (the latter also corrects two claims in the former's message; the description carries the definitive squash message): - **The FNFE-vs-IOException split is pinned on both sides**: a non-missing-file IO error escapes as `HoodieIOException` and keeps the last known owner rather than clearing it. - **`checkIfExpired`'s catch had never executed.** A stub failing only `getPathInfo` pins that a stat failure cannot steal a live lock (the holder's on-disk payload survives byte-identical) and cannot skip the owner-info reload. The fail-safe-toward-the-holder choice matches `StorageBasedLockProvider`'s `UNKNOWN_ERROR` arm and is documented; the catch also logged `ALREADY_RELEASED`, the opposite of what it concludes, and logs `ACQUIRING` now. - `initLockInfo` is synchronized (the lazy fields lost `final`'s free safe publication); `currentOwnerLockInfo` initializes to `""` matching the `LockProvider` interface default, so `LockManager` never logs a `null` owner. Every pin was reverted individually to confirm it fails exactly the assertion written for it. Transaction package: 262 tests, 0 failures; checkstyle and rat clean on both touched modules. The serialVersionUID paragraph in the description should be replaced by a note that the class has not been Java-serializable at all since 0.13.0. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
