voonhous commented on PR #18814: URL: https://github.com/apache/hudi/pull/18814#issuecomment-5316716537
Third review round. It converged -- no new correctness findings against the diff, and the two remaining items were a wrong javadoc rationale and a missing operational signal. Both fixed in `b45d3d5`. **The javadoc was justifying the wrong thing.** It said inner consecutive slashes and scheme spelling are left alone because collapsing them "could map unrelated tables onto one lock". That is false: `HoodieTableMetaClient:196` wraps the base path in a `StoragePath`, which collapses both, so `s3://b/x//t` and `s3://b/x/t` are the *same table* and still derive *different* lock keys. Verified against the real `StoragePath`: ``` StoragePath: s3://b/x//t vs s3://b/x/t -> same=true lock key: s3://b/x//t vs s3://b/x/t -> different ``` That is the same bug class this PR fixes, left in place. It is not a regression -- the gap predates the PR and this change is a strict improvement -- but it is a known residual gap, not a safety choice, and the comment now says so. Worth a follow-up ticket alongside the `InProcessLockProvider` one. URL encoding stays a genuine deliberate exclusion. **Nothing signalled when a lock key actually moved.** Both providers logged raw and normalized at INFO on every construction, so a writer whose key moved -- the population that must not do a rolling upgrade -- looked identical in the logs to one that was fine. Both now emit a WARN naming the old and new key, firing exactly when the canonical form differs from the pre-change `s3aToS3` form. It correctly stays silent for `s3a://` inputs, whose key did not move. **Verification this round.** The three test classes were compiled verbatim and executed outside maven: 34/34 provider assertions and 44/44 `TestFSUtils` assertions pass, independently re-confirming `C0E15D0CE1AD11CC` against the real lz4 XXH64. Idempotence fuzzed again over the final implementation: 0 non-idempotent, 0 outputs that the function then rejects. Checkstyle 9.3 clean under both this branch config and master. Also correcting something I implied earlier: checkstyle **is** enforced in CI, in `test-spark-client-and-hadoop-common` (covers hudi-common, hudi-client-common, hudi-hadoop-common via `-am`) and `test-utilities` (covers hudi-aws). Azure skips it globally, which is what misled me. All four test classes are executed by real lanes -- `UT_MODULES` and `job10UTModules` are exclusion lists and exclude neither hudi-aws nor hudi-client-common. Remaining before merge, both outside the diff: 1. Green CI on `b45d3d5`. Nothing has been green on any of these commits yet; the last SUCCESS is the June run on `3b4db31`, on a job matrix that no longer exists. 2. The asf-site Breaking Changes entry plus a tracking ticket, per the #12220 precedent. `release-1.2.md` already has that section to model on. -- 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]
