voonhous commented on code in PR #18814:
URL: https://github.com/apache/hudi/pull/18814#discussion_r3796761109
##########
hudi-common/src/main/java/org/apache/hudi/common/fs/FSUtils.java:
##########
@@ -759,6 +759,31 @@ public static String s3aToS3(String s3aUrl) {
return s3aUrl.replaceFirst("(?i)^s3a://", "s3://");
}
+ /**
+ * Canonicalize a Hudi table base path for use as the input to implicit
lock-key derivation.
+ *
+ * <p>Implicit lock providers (DynamoDB and Zookeeper variants) hash this
string to choose the
+ * lock row / znode for a table. Two callers writing to the same table must
produce the same
+ * hash, so any benign formatting drift in the basePath has to be eliminated
before hashing.
+ * This method trims surrounding whitespace, normalizes s3a:// to s3://,
then forces exactly
+ * one trailing slash. Inner double slashes are intentionally preserved.
+ */
Review Comment:
Half of this is now obsolete, half is still the live question, so recording
both rather than resolving.
**Obsolete:** the premise "the canonical form now always has a trailing
slash" stopped being true at `3b4db31`. The canonical form *strips* trailing
slashes and appends nothing, so a base path without a trailing slash -- the
common case -- hashes to exactly what previous releases hashed,
`hash(s3aToS3(basePath))`. There is nothing to preserve as a fallback for those
users because nothing moved. That is now pinned by a test in a lane that runs
(`basePathWithoutTrailingSlashKeepsThePreNormalizationKey` / `...Znode`),
rather than only by the `@Disabled` IT.
**Still live, and this is the one for @yihua:** the release-note callout.
Base paths *with* a trailing slash or surrounding whitespace do move to a new
lock row / znode, and those tables cannot be upgraded one writer at a time.
Separately, base paths that reduce to nothing lockable (`""`, `"/"`, `"s3://"`,
`"file:///"`) now fail closed with `IllegalArgumentException` where they
previously hashed to a working key.
The precedent is #12220, where nsivabalan asked for a website/runbook update
plus a tracking ticket *if* the provider had already shipped. It had not then.
It has now -- this scheme is in 1.0.0 through 1.2.0, so this PR is the first to
move a released lock key. A hard cutover matches precedent (there is no
mechanism to bridge it: a lock key is never persisted, so an upgrade handler
cannot help), but the note is missing.
As of `b45d3d5` both providers also emit a `WARN` naming the old and new key
whenever the canonical form differs from the pre-change one, so operators can
actually detect the affected tables instead of inferring them. Leaving this
thread open until there is a decision on the asf-site entry and a tracking
ticket.
--
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]
