nsivabalan commented on PR #19033: URL: https://github.com/apache/hudi/pull/19033#issuecomment-5222221497
@danny0405 this is ready for another look when you have time. **Since your last review, the branch was rebased onto master.** That mattered more than usual here: #19391 landed the same class of fix in `HiveDriverPool` that we were making in this PR, and since this PR *extracts* that inner `Dispatch` class into a shared `ParallelDispatch` (so the DROP Thrift pool can reuse it), the two collided. Resolution keeps the extraction and folds your/Vova's refinements into `ParallelDispatch`. Branch is `MERGEABLE` again and CI is green. Worth calling out: **#19391 independently confirms the race described in the P3 thread below** — its `recordFailure` javadoc documents exactly the `cancelPending()`-wins-the-`FutureTask`-CAS hazard that was measured on this branch. **The one thing genuinely waiting on you: P3 on `ParallelDispatch:190`** (the double-counted cancellations). It is still deferred, and the reasoning is in that thread: fixing the count naively removes what currently *masks* that race, turning a real metastore exception into a bare `CancellationException`. Measured, not theorized: ``` pre-fix FAIL=EXEC:IllegalStateException count 4 (wrong) failure PRESERVED naive P3 FAIL=BARE_CANCEL count 2 (right) failure DESTROYED ``` Four variants were tried and rejected. A correct fix means reworking the `abort()` / `taskSettled()` / `signalIfComplete()` wake protocol — the same machinery implementing the abort-on-first-error guarantee — so it seemed to deserve its own PR rather than riding along here. Happy to do it either way; just want your call. **Addressed since you last looked:** | Fix | What | |---|---| | `97dc86a` | An `Error` that aborted a dispatch was reported twice — as root cause *and* suppressed. `guard` catches `Throwable`, so `asException()` wrapped it and the identity check could never match. Now compares the raw cause. | | `3a1694a` | **The one I would most like your eyes on.** DROP was the only sync path invoking a user-supplied `PartitionValueExtractor` from several pool workers at once. Extractors are pluggable with no thread-safety contract, so a stateful one could return a garbled clause and drop the *wrong* partition. Values are now resolved on the calling thread before fan-out. A test recording call threads fails on the old code with all four workers inside the extractor. | | `4d0ac71` | Renamed `IMetaStoreClientPool` → `HiveMetaStoreClientPool` (the `I` prefix read as an interface next to the real `IMetaStoreClient`). | | `038e6c8` | Test-only: `TestHoodieHiveSyncClientClose` uses `CALLS_REAL_METHODS`, which skips the constructor, so the `partitionClientPool` field this PR adds was null and `close()` NPE'd before reaching the client — surfacing as a misleading "zero interactions with this mock". | Current size is 12 files, +1273/−255. No new config keys — everything reuses the batching configs from #18984, and `batching.enabled=false` (the default) is byte-identical to pre-PR behaviour. -- 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]
