perf(timeline-service): avoid refreshing exact timeline extensions - #19946
goutamadwant wants to merge 3 commits into
Conversation
Compare the server timeline prefix through the client boundary before refreshing an already newer view. Preserve conservative refreshes for missing boundaries and divergent action or state histories. Add HTTP and file-backed view regressions for issue apache#19360, including repeated requests, out-of-order completion and necessary refreshes.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for working on this! The PR makes RequestHandler#isLocalViewBehind treat the server view as up to date when the server timeline is an exact extension of the client's (matching (requestedTime, action, state) hash through the client's last-known instant), avoiding repeated sync()/timeline reloads and partition-view rebuilds. I traced the new branch against server-behind, archived-prefix, out-of-order completion, action/state drift, pending compaction/log-compaction filter asymmetry, rollback/restore, and the mid-request sync race, and the implementation matches the exact-prefix design @danny0405 endorsed on #19360; the test matrix covers those cases well. One question inline about the behavioural shift for callers that previously hit the BadRequestResponse → PriorityBasedFileSystemView local-fallback path. Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
cc @yihua
| } | ||
| // 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. | ||
| return !localTimeline.findInstantsBeforeOrEquals(lastKnownInstantFromClient) |
There was a problem hiding this comment.
🤖 One behavioural shift worth calling out explicitly: before this change, a server that was strictly ahead of the client (beyond a trailing clean) failed the final check with BadRequestResponse, which PriorityBasedFileSystemView turns into a sticky fallback to the local secondary view — i.e. the caller got results computed from its own timeline. Now the server answers from its newer timeline instead. For bounded calls (...BeforeOrOn) that's equivalent, but unbounded write-path callers (HoodieAppendHandle.getLatestFileSlice, HoodieAbstractMergeHandle.getLatestBaseFile, SparkInsertOverwrite*.getLatestFileSlices, ConsistentBucketIndexUtils.getLatestBaseFiles) can now observe file slices from instants the client hasn't seen. I believe that's the intended semantics from the issue discussion, but could you confirm it was considered, and maybe note it in the isLocalViewBehind javadoc? The trailing-clean special case in shouldThrowExceptionIfLocalViewBehind is also now subsumed by this branch, so a short comment there tying the two together might help future readers.
There was a problem hiding this comment.
Confirmed: exact extensions now stay on the server view instead of triggering local fallback. Added documentation and regressions for bounded/unbounded reads, pending compaction, and divergent-timeline fallback.
One distinction: bounded results are not always equivalent to the client’s view, since newer pending compaction can exclude a file group despite the same timestamp limit. The follow-up leaves the runtime algorithm unchanged. All 150 broader tests passed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #19946 +/- ##
============================================
- Coverage 80.23% 80.21% -0.02%
Complexity 34747 34747
============================================
Files 2546 2546
Lines 142608 142646 +38
Branches 17362 17365 +3
============================================
+ Hits 114419 114425 +6
- Misses 20277 20305 +28
- Partials 7912 7916 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Document that exact extensions serve the current server view without local fallback, including pending-compaction state in bounded queries. Add real remote and priority-view regressions for bounded and unbounded selection, pending compaction, and sticky fallback after timeline divergence. Relates to apache#19360.
| 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. |
There was a problem hiding this 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.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback! The new commit adds a javadoc to isLocalViewBehind spelling out that an exact extension is accepted by both the refresh and final checks (and therefore no longer triggers the PriorityBasedFileSystemView error fallback), cross-references that from the trailing-clean carve-out in shouldThrowExceptionIfLocalViewBehind, and adds three end-to-end tests through PriorityBasedFileSystemView covering exact-extension serving, bounded queries under a server-side pending compaction, and the retained sticky fallback for a divergent timeline. That resolves my earlier question. The predicate logic itself is unchanged from what I traced last time, and I re-checked the new tests: the remote view snapshots the client timeline at construction, so the extension path is genuinely exercised rather than a same-hash no-op, and the divergent-timeline test correctly pins one sync(), one server listing, and two local listings.
One thread remains open: @danny0405's P1 (posted after this commit landed) asks to keep exact-prefix acceptance out of the final consistency check so that the previous BadRequestResponse → local-fallback behaviour is preserved, with only the refresh decision relaxed. The new testExactExtensionServesCurrentFilesWithoutFallback and testBoundedSelectionUsesServerPendingCompaction (plus the earlier testExtensionPassesInitialAndFinalChecks) encode the current design and would need to flip if that suggestion is adopted, so it's worth settling that design question before iterating further on tests. Azure CI for eed25903 is still pending. No new issues flagged from this automated pass — a Hudi committer or PMC member can take it from here for a final review.
danny0405
left a comment
danny0405
left a comment
There was a problem hiding this comment.
let's be conservative before reaching consensus on the corner cases.
hudi-agent
left a comment
hudi-agent
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround on the feedback! This commit splits the refresh decision out into shouldRefreshLocalView (exact-extension aware) and restores isLocalViewBehind — the final consistency check — to be byte-identical with master apart from the comment, so a server-ahead response is rejected again exactly as before (trailing-clean carve-out included). That addresses @danny0405's P1 and the earlier behavioural-shift thread from the first pass; @goutamadwant's reply described the previous design and is superseded. The unit and e2e tests were flipped to match, and the full hudi-timeline-service suite passes locally (99 tests, JDK 17).
One new problem, though, and it's the reason Azure is red on fe2dfa38: TestSavepointRestoreMergeOnRead#testCleaningDeltaCommits and #testRestoreWithFileGroupCreatedWithDeltaCommits (table version [1]) now fail 4/4 reruns with an HTTP 400 from RemoteHoodieTableFileSystemView.getLatestFileSlicesBeforeOrOn inside ListingBasedRollbackStrategy, while the two earlier commits were green. The premise in the new javadoc — that a reload can't make the server timeline equal the older client timeline — doesn't hold when instants are deleted from storage: restore's per-commit rollbacks run with skipTimelinePublish=true and remove completed commit instants, so the executor's freshly reloaded timeline becomes an exact prefix of the server's now stale-ahead in-memory timeline. Master syncs there and the server catches back up; this PR skips the sync, and the (correctly) strict final check then rejects the request. Details and a suggested narrowing in the inline comment. (The other failing Azure job, "UT spark-datasource Java Test 2", is just a telemetry-publish timeout after BUILD SUCCESS.)
Please take a look at any inline comments, and this should be ready for a Hudi committer or PMC member to take it from here.
| /** | ||
| * Determines whether the local view needs to be refreshed before handling the request. | ||
| * | ||
| * <p>An exact extension does not require a refresh because reloading cannot make the |
There was a problem hiding this comment.
🤖 This premise doesn't hold when instants get deleted from storage, and I think it's what's failing Azure on this commit: TestSavepointRestoreMergeOnRead#testCleaningDeltaCommits / #testRestoreWithFileGroupCreatedWithDeltaCommits fail 4/4 reruns with a 400 from getLatestFileSlicesBeforeOrOn in ListingBasedRollbackStrategy (earlier commits were green). Restore's per-commit rollbacks run with skipTimelinePublish=true and delete completed commit instants, so the executor's reloaded timeline is an exact prefix of the server's now stale-ahead view; master would sync here and catch back up, whereas this skips the sync and the strict final check rejects. Since the final check only ever serves a non-equal timeline when the extension is a trailing clean, could the skip be narrowed to that case (e.g. reuse shouldThrowExceptionIfLocalViewBehind on the prefix)? For any other extension master syncs-then-throws anyway, so skipping buys nothing there but loses the deleted-instant recovery.
This branch has not been deployed
5 participants
Describe the issue this Pull Request addresses
Closes #19360.
A remote filesystem view retains its client timeline between requests. When the server already contains that timeline plus newer instants, unequal full hashes repeatedly trigger a sync that reloads the same timeline and discards cached partition views.
Summary and Changelog
Implement the exact-prefix approach proposed and discussed in the issue. On a full-hash mismatch, require a valid client boundary present on the server and compare the server timeline through that boundary with the client hash. Skip refresh only when the hashes match.
Keep the existing filtering and hashing rules, full-hash equality check, synchronization, and conservative refresh for divergent or archived prefixes. The same decision serves the initial refresh and post-request consistency checks.
Add HTTP regressions for exact extensions, boundary validation, archival, out-of-order completion, action/state differences, filtering, and changes during request handling. Add real file-backed view tests for repeated remote file-slice requests and refreshing before returning a newer base file.
Document that accepting an exact extension also avoids the final error-based fallback. Add real remote/priority-view coverage for bounded and unbounded file selection, plus a divergent-timeline control for sticky fallback.
Impact
Avoid unnecessary timeline reloads and partition-view rebuilds when the server is a proven extension of the client timeline. No request protocol, public API, configuration, or storage-format changes.
Unbounded latest-file requests remain on the server's current view and can return files from instants newer than the client's timeline, instead of falling back to its local secondary view. Explicitly bounded requests retain their supplied instant limit but use the server's current pending-compaction state. A newer pending compaction can exclude a file group when
includeFileSlicesInPendingCompactionis false, so bounded results are not necessarily equivalent to the client's local view.In the local file-backed regression, 32 remote file-slice requests across eight warmed partitions went from 32 syncs, 32 timeline reloads, and 64 directory listings to zero of each, with the same expected file selections. This is operation-count evidence, not an end-to-end workload speedup measurement.
Risk Level
High: filesystem-view refresh decisions affect file selection. The optimization retains the complete prefix hash, including requested time, action, and state, rather than using the latest timestamp alone. Divergence and out-of-order completion controls verify that necessary refreshes remain in place.
Focused follow-up validation passed all 17 tests in
TestTimelineViewRefreshandTestRequestHandler, including real remote/priority-view selection with an ordinary commit extension and a pending-compaction extension, and preserved sticky fallback after divergence.Broader follow-up validation passed all 99 timeline-service tests, including 54 remote filesystem-view tests, plus 51 shared filesystem-view tests. Checkstyle and RAT passed. These local tests do not replace a production HDFS/Spark/Flink row-level workload replay.
Documentation Update
Clarify exact-extension visibility and its relationship to the existing trailing-clean exception in
RequestHandler.Contributor's checklist