nsivabalan opened a new pull request, #20239: URL: https://github.com/apache/hudi/pull/20239
perf(timeline-service): skip reloads already performed for a stale client timeline ### Describe the issue this Pull Request addresses Builds on #19946 by @goutamadwant, whose commits are carried here so this applies to master on its own. That PR removes the redundant reload for the trailing-clean case; this adds the general form of the same fix. Credit for the trailing-clean work and its test suite belongs to that PR — if it merges first, this rebases down to just the cache commit. A reload of the server's file system view is only useful when the server is behind storage. The refresh check fires on timeline hash inequality, which conflates three different situations: | Situation | Reload helps? | Today | |---|---|---| | Server stale, client ahead | yes — it converges | correct | | Client stale, server ahead on the same lineage | **no** — it lands on the same state | reload on every request, indefinitely | | Divergence, e.g. restore deleted instants | yes — the server is the stale one | correct | Rows 1 and 3 heal after one reload: the next request matches and skips. Row 2 never heals. The stale executors are pinned to the timeline they loaded at task start, and nothing the server does changes their hash. So every request from every stale executor triggers another full reload, each one serialized under `synchronized (view)`, each one landing on the state it started from. This is reachable whenever a concurrent writer advances the timeline mid-stage. @prashantwason reported the trailing-clean variant from production on #19946: a COPY_ON_WRITE upsert with ~1000 executors where an async clean completed mid-write, the timeline server stopped responding, marker creation failed with `NoHttpResponseException` after ~10 minutes, and the stage aborted after all task retries. ### Summary and Changelog The first eight commits are from #19946 unchanged, including its revert pair, which left the exemption at a single trailing clean. The final commit is the new work described below. Record the client timeline hash and last known instant that a reload was performed for, together with the server timeline hash that reload produced. Skip the reload for a later request carrying the same pair while the server timeline is still that hash. - `RequestHandler` keeps a bounded per-table map of (client hash + last known instant) to the server timeline hash observed after the reload. It is read and written under the per-table view lock that `syncIfLocalViewBehind` already holds. - A recorded entry is honored only while the server's current timeline hash still matches the recorded one. Any change to the server timeline, for any reason, makes a reload potentially useful again and the entry stops applying. - A reload that drops instants, which is what a restore does, discards that table's records rather than reasoning about which of them survived. - Nothing is recorded when the client sent no last known instant. That only happens before the client's timeline holds a completed instant, where there are no file slices to serve from a stale view. - The record is also cleared on the explicit refresh-table endpoint, since that rebuilds the view from storage. - New config `--stale-client-timeline-cache-size`, default 10, set to 0 to disable. The trailing-clean exemption from #19946 is also moved ahead of `isLocalViewBehind`, which hashes the whole timeline, since the exemption only inspects the last instant. **What is deliberately not done here.** Only an observed reload is treated as evidence. The client's hash is never interpreted as proof that the server is ahead of it, and no attempt is made to infer an ancestor relationship from the hash. That inference is what made the earlier general-extension attempt on #19946 (81ba62a) unsafe: an exact-prefix match establishes containment but not that results from the server's newer view are interchangeable with the client's, and it broke `TestSavepointRestoreMergeOnRead` because restore deletes completed instants and leaves the server stale-ahead on what looks like a clean prefix. Response behavior is unchanged. This skips a reload that could not have changed the outcome; it does not change what the final consistency check does or what is returned. A client that is genuinely behind still fails that check, still gets its 400, and still falls back to its local view through `PriorityBasedFileSystemView`. ### Impact No public API, configuration semantics, request protocol, storage format, or file-selection change. One new server-side config with a safe default. The benefit is workload-shaped rather than universal. An entry survives only between real timeline changes, so a table whose timeline advances constantly will invalidate often and gain little — which is correct, since frequent change means reloads are not redundant. The win is exactly in the case this targets: a long-running write with stale concurrent readers, where the server timeline is quiet and the reloads are pure waste. ### Risk Level medium Refresh decisions affect read and write paths. Mitigations: - All 107 `hudi-timeline-service` tests pass, including all 54 `TestRemoteHoodieTableFileSystemView` tests, both before and after rebasing onto master. **No existing test was modified**, including the 23 from #19946. - `TestSavepointRestoreMergeOnRead` passes 18/18. This is the suite that failed 4/4 on 81ba62a, the earlier attempt at this optimization, so it is the specific regression this design had to clear. - New tests cover: many stale clients sharing a single reload, a server timeline change still forcing a reload for a known client hash, deleted instants dropping recorded reloads, and bounded eviction. - Checkstyle clean. Two existing tests caught design errors during development and are worth noting as evidence the guards are load-bearing. `testDivergentPrefixesStillSync` caught an earlier version that keyed only on the client hash: it iterates several divergent server timelines against one client hash, and suppression after the first reload produced a 400 where master recovers. That is what the server-hash guard exists for. `testInvalidOrMissingBoundaryDoesNotProveExtension` caught that the refresh decision also depends on the client's last known instant, not the hash alone. ### Documentation Update New config `--stale-client-timeline-cache-size` on the timeline server, documented inline. No website update needed. ### Contributor's checklist - [x] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute) - [x] Enough context is provided in the sections above - [x] Adequate tests were added if applicable 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
