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] Keep exact-prefix acceptance out of the final consistency check
This predicate is used both to decide whether to sync and to validate the
response after handling it. A prefix match proves that the server contains the
client's timeline, but accepting it in the final check also lets the request
return files outside that timeline and suppresses the existing
`PriorityBasedFileSystemView` fallback.
I reproduced the difference with real files and
`PriorityBasedFileSystemView` on `81ba62a`: a client captures `001` and reads
partition A, then `002` updates both A and B and the shared server view is
synced. Without refreshing the client, reading B returns `002` with this
branch; with the pre-PR handler, the same test passes an expectation of `001`
through the local fallback. The client can therefore mix snapshots across
partition requests. Unbounded write-path lookups such as
`HoodieAbstractMergeHandle.getLatestBaseFile` can also observe files newer than
the timeline used for record tagging.
The new Javadoc describes the change, but does not preserve the previous
behavior. The new `testBoundedSelectionUsesServerPendingCompaction` also
demonstrates that supplying an explicit time bound does not always preserve the
local result: the remote call returns zero slices where the client view returns
one.
Could we separate the refresh decision from final response validation? A
minimal change is:
```java
private boolean isLocalViewBehind(Context ctx, boolean allowExactExtension) {
// Existing parameter extraction, timeline filtering, and empty-timeline
check.
String localTimelineHash = localTimeline.getTimelineHash();
if (!localTimelineHash.equals(timelineHashFromClient)) {
if (!allowExactExtension
||
HoodieTimeline.INVALID_INSTANT_TS.equals(lastKnownInstantFromClient)
|| !localTimeline.containsInstant(lastKnownInstantFromClient)) {
return true;
}
return
!localTimeline.findInstantsBeforeOrEquals(lastKnownInstantFromClient)
.getTimelineHash().equals(timelineHashFromClient);
}
return
!localTimeline.containsOrBeforeTimelineStarts(lastKnownInstantFromClient);
}
```
Use `isLocalViewBehind(ctx, true)` in `syncIfLocalViewBehind`, and
`isLocalViewBehind(context, false)` in the final check. Keep the existing
`shouldThrowExceptionIfLocalViewBehind` handling, including its trailing-clean
exception. This skips redundant reloads while retaining the previous response
validation and fallback behavior. Tests that require accepting arbitrary
extensions in the final check would need to change accordingly.
--
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]
