danny0405 commented on code in PR #19946:
URL: https://github.com/apache/hudi/pull/19946#discussion_r4011773731
##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -750,9 +757,15 @@ private boolean isLocalViewBehind(Context ctx) {
}
String localTimelineHash = localTimeline.getTimelineHash();
- // refresh if timeline hash mismatches
if (!localTimelineHash.equals(timelineHashFromClient)) {
- return true;
+ if
(HoodieTimeline.INVALID_INSTANT_TS.equals(lastKnownInstantFromClient)
+ || !localTimeline.containsInstant(lastKnownInstantFromClient)) {
+ return true;
+ }
+ // A newer last instant alone is insufficient: all actions and states
through the
+ // client boundary must match before the server can be treated as an
exact extension.
Review Comment:
[P1] Do not merge this optimization with relaxed final consistency validation
I do not think we should merge this patch as written. An exact-prefix hash
establishes that the server contains the client's timeline, but it does not
establish that results computed from the server's newer view are
interchangeable with results from the client's view. Using this predicate for
both refresh and final validation changes file-selection behavior, rather than
just avoiding redundant reloads.
**Why the final check should stay unchanged**
The client retains its timeline between requests, and callers can already
have made decisions using that timeline, such as tagging records with
file-group locations. Returning a newer server view does not refresh those
decisions. For example, unbounded write-path calls such as
`HoodieAbstractMergeHandle.getLatestBaseFile` can observe a newer base file or
a file group that has since been replaced. A matching prefix alone does not
establish that those results are compatible with the caller's earlier work.
The existing post-request check rejects timeline mismatches, subject to the
existing trailing-clean exception. For `PriorityBasedFileSystemView`, that
rejection activates the local secondary view based on the client's timeline.
Checking after request handling also detects a timeline change during handling.
Accepting arbitrary extensions here removes that rejection/fallback path even
though the caller's state has not been refreshed. We should preserve this
existing behavior unless there is caller-level evidence that relaxing it is
safe.
**Observed behavior**
I reproduced the change with real files and `PriorityBasedFileSystemView` on
`81ba62a`: the client captures `001` and reads partition A; then `002` updates
A and B and the shared server view is synced. Without refreshing the client,
reading B returns `002` with this patch. With the pre-PR handler, the same test
returns `001` through the local fallback. Thus one client can receive A from
`001` and B from `002`. This demonstrates changed snapshot behavior; it is not
an end-to-end data-corruption reproduction.
The current `testBoundedSelectionUsesServerPendingCompaction` also
demonstrates that an explicit time bound is not sufficient to preserve the
client's result: the remote call returns zero slices where the local view
returns one. Documenting and testing these new results does not establish that
the callers consuming them remain correct.
**Recommendation**
Keep the final consistency check and its existing exception handling
unchanged, and do not accept this patch's general-extension optimization
without validating the affected read/write callers. Separating refresh from
final validation would retain gains for the existing trailing-clean case, but
other extensions would still be rejected and fall back; that is a narrower
optimization, not a solution to the broader goal proposed here. A follow-up
limited to eliminating redundant refreshes while preserving response behavior
would be easier to assess.
--
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]
