nsivabalan commented on PR #19946:
URL: https://github.com/apache/hudi/pull/19946#issuecomment-6070719946
# Review draft — apache/hudi#19946
## Mergeability summary
- Narrow, well-scoped change: 5 lines of production code reusing an existing
predicate. No new algorithm, no new state, no config.
- No breaking changes. No storage format, public API, config semantics, wire
protocol, or file-selection changes.
- The optimization is confined to the mismatch the final consistency check
already tolerates, so response behavior is unchanged for every other case.
- The symmetry is the strongest property here: the sync decision and the
reject decision now come from the same function on the same (unfiltered)
timeline, so they cannot drift.
- Test coverage is thorough — 23 tests including concurrent stale clients,
file-backed operation counts, deleted-commit recovery, and local-fallback
preservation.
- History shows the broader version of this optimization was tried
(81ba62a), P1'd, and broke `TestSavepointRestoreMergeOnRead` in CI. The current
narrow form is the right landing spot.
**Recommendation: approve**, with one non-blocking ordering suggestion.
## Breaking changes
None.
## Findings
### SUGGESTION — hoist the predicate above `isLocalViewBehind` (sync site
only)
`RequestHandler.java:770-780`. Current shape enters a block named "view is
behind" and then returns without syncing, which reads as a bug. Inverting also
avoids wasted work: `isLocalViewBehind` builds a filtered timeline and computes
a SHA-256 over the full instant list (`:742`, `:752`), while
`shouldThrowExceptionIfLocalViewBehind` only inspects `lastInstant()`. In the
trailing-clean case the current order pays for the hash and discards the answer.
```java
HoodieTimeline localTimeline = view.getTimeline();
if (!shouldThrowExceptionIfLocalViewBehind(localTimeline,
getTimelineHashParam(ctx))) {
return false;
}
if (isLocalViewBehind(ctx)) {
...
view.sync();
return true;
}
```
**Important caveat: do not mirror this at the final check (`:692-702`).**
The two predicates read different timelines — `isLocalViewBehind` uses
`getTimeline().filterCompletedOrMajorOrMinorCompactionInstants()` (`:742`),
while `shouldThrowExceptionIfLocalViewBehind` uses the unfiltered
`getTimeline()` (`:695-696`). `shouldThrow...` returns `true` for essentially
every healthy request whose last instant is not a clean, so promoting it to the
outer condition there would 400 on matching timelines. `isLocalViewBehind` is
the guard that gives it meaning.
### SUGGESTION — note the filter asymmetry
Pre-existing, not introduced here, but the new call site inherits it: the
"detect" step hashes a filtered timeline and the "forgive" step hashes an
unfiltered one. Worth a comment so the next reader does not assume they are
interchangeable. `testCompactionStateAndFiltering` already covers the
log-compaction case where the two disagree. Aligning both on one filtered
timeline would be cleaner but has its own blast radius and should not ride
along on this PR.
### NIT — method name
`shouldThrowExceptionIfLocalViewBehind` now also gates refreshes, so the
name undersells it. Something like `isBenignTimelineDifference` would read
better at both call sites. Not worth churn if there is no other reason to touch
the method.
## On the multi-clean discussion
Two reviewers are currently pulling in opposite directions and neither has
replied to the other:
- @prashantwason (2026-10-08) proposes extending the exemption to walk back
over all trailing cleans, with a reproducing test.
- @danny0405 (2026-10-08) asked for exactly that implementation to be
reverted as "a rare case" — which the author did in `e2800c2` / `c7c713d`.
Worth resolving explicitly rather than leaving the next reader to guess. The
narrow form is defensible: a second concurrent clean completing inside one
stage is genuinely uncommon, and the complexity is real. If the production
evidence says otherwise, that is the argument to make.
## Suggested follow-up (not for this PR)
The remaining cost is the general stale-client case: when a concurrent
writer leaves N executors pinned to an older timeline, every request reloads,
each reload lands on the same storage state, and nothing converges — reloads
are only useful when the server is behind storage. This is the trailing-clean
problem's general form and is not addressed here.
I have opened #20239 for that, which carries the commits from this PR so it
applies to master on its own. The server records the client hash and last known
instant a reload was performed for, together with the server timeline hash that
reload produced, and skips the reload while that server timeline is unchanged.
Only an observed reload counts as evidence — the client hash is never
interpreted as proof the server is ahead, which is what made 81ba62a unsafe
here. Validated with all 107 timeline-service tests and
`TestSavepointRestoreMergeOnRead` at 18/18, the suite that failed 4/4 on
81ba62a, with no existing test modified.
If this PR merges first, #20239 rebases down to just the one commit. Happy
to close it in favour of folding the change in here instead, whichever the
reviewers prefer.
--
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]
