voonhous commented on PR #18814:
URL: https://github.com/apache/hudi/pull/18814#issuecomment-5316337329
Ran a second review round, this time against my own commits rather than the
original diff. It found two build blockers I had introduced, plus one real
correctness gap. All fixed in `8ad0ed1`.
**Build blockers (mine, now fixed).** Two of the test methods I added were
named `aBasePathWithoutTrailingSlash...`, which violates checkstyle's
`MethodName` pattern `^[a-z][a-z0-9][a-zA-Z0-9_]*$`. Checkstyle is bound to the
`compile` phase with `failOnViolation=true` and
`includeTestSourceDirectory=true` (root `pom.xml:308-343`), so `hudi-aws` and
`hudi-client-common` would not have built. Renamed.
Separately, this file's import block predates #18886, which both fixed the
ordering and raised `ImportOrder` to `severity=error`. The PR did not touch
those lines so a merge is fine, but the file would fail checkstyle the moment
anyone rebased. Restored to match master.
Both verified with checkstyle 9.3 run directly against
`style/checkstyle.xml` and against master's stricter copy: 0 violations across
all 7 changed files.
**The idempotence fix was incomplete.** I had changed the strip loop to
`Character.isWhitespace`, but the `trim()` immediately above it uses `c <= '
'`, and those are different sets -- `U+0000`-`U+0008` and `U+000E`-`U+001B` are
stripped by `trim` and not by `isWhitespace`. So a path ending in one of those
followed by a slash still normalized to a value that normalized further, and in
some cases to a value the function then *rejected*, which breaks the "callers
may pass an already-normalized value back through" contract both providers
document. The loop now uses `c <= ' '` so it matches `trim` exactly.
Fuzzed to confirm: all inputs up to length 4 over an alphabet of slash,
every whitespace class, control characters, colon and letters -- 44266
accepted, **0 non-idempotent, 0 that reject their own output**. Control-char
cases added to `TestFSUtils#testNormalizeBasePathForLockingIsIdempotent`.
**Documentation gaps closed.** The reject rule applies to scheme roots for
*every* scheme, not just s3 -- `file:///`, `hdfs:/`, `gs:///` are all rejected,
and none of them were documented or tested. More importantly, that is a genuine
behaviour change and the description did not say so: those inputs previously
hashed to a working lock key and now fail the writer at construction. "Fails
closed" is a different risk class from "key moves", so it is now its own bullet
in Compatibility, with the reject list added to the test.
I also documented the trailing-whitespace trade-off explicitly: a base path
differing from another only by trailing whitespace now shares a lock. That
collapse comes from the pre-existing `trim()`, not from anything added here,
and over-serializing two such tables is the safe direction to err.
**One thing worth adding to the description, which I did.** This is not a
hypothetical cross-engine problem -- Spark already contradicts itself within a
single writer. `HoodieSparkSqlWriter` passes the raw `optParams("path")` on the
upsert/insert/delete paths (`:413`, `:452`, `:502`, `:795`) but
`basePath.toString` on the row-writer bulk-insert path (`:832`), where
`basePath = new Path(path)` (`:219`) has already had its trailing slash
stripped by Hadoop's `Path`. I confirmed with a real
`org.apache.hadoop.fs.Path` that `new Path("s3://bucket/table/").toString()` is
`"s3://bucket/table"`. So `save("s3://bucket/table/")` derives one lock key for
an upsert and a different one for a bulk insert, on the same table, on master
today. That seems like the strongest single argument for merging this.
**Two things I did not change.**
The DynamoDB provider's *constructor* is still exercised nowhere in CI --
the new tests only call the static `derivePartitionKey`, and the only
constructing test is the `@Disabled` IT.
`DynamoDBBasedLockProviderBaseTest:65-70` has a mock-`DynamoDbClient` pattern
that would close this if you want it; I left it alone rather than grow the diff
further. The ZK constructor is covered.
Also `TestHashID.java:125` does `expectedValuesMap.put(HashID.Size.BITS_128,
hash64ExpectedValues)` where it means `BITS_64`, so it is overwritten at `:137`
and the six pinned BITS_64 vectors are never asserted. Those literals also
encode the unsigned seed rather than the sign-extended `0xFFFFFFFFDABADABA`, so
they disagree with the live `hash/xxhash_BITS_64_for_magic_input.txt` fixture.
Pre-existing and unrelated to this PR, but worth a separate minor ticket,
especially since this PR is the first live pin of a short-input BITS_64 hash.
**Merge mechanics.** Merge into master is clean. A *rebase* is not -- an
intermediate commit here touched `InProcessLockProvider.java`, which #19193
turned into a compat alias, so replaying commit-by-commit conflicts even though
the final tree does not touch that file. Squash first if a committer wants a
rebase rather than a squash-merge.
Still not run: the build. The ZK contention test's behaviour was reproduced
against a real curator `TestingServer` outside the suite (writer B times out at
~1032ms and its ephemeral node is cleaned up; on master's derivation both
writers acquire, so the assertion does discriminate), but the suite itself has
not been executed.
--
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]
