You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "nsivabalan (via GitHub)" <gi...@apache.org> on 2026/10/08 22:57:20 UTC
[PR] perf(timeline-service): skip reloads already performed for a stale client timeline [hudi]
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(timeline-service): skip reloads already performed for a stale client timeline [hudi]
Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
hudi-agent commented on code in PR #20239:
URL: https://github.com/apache/hudi/pull/20239#discussion_r4235357552
##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java:
##########
@@ -134,6 +134,13 @@ public static class Config implements Serializable {
@Parameter(names = {"--enable-remote-partitioner"}, description = "Enable remote partitioner")
public boolean enableRemotePartitioner = false;
+ @Builder.Default
+ @Parameter(names = {"--stale-client-timeline-cache-size"}, description =
+ "Number of client timeline hashes to remember for which a refresh has already been performed. "
+ + "When many executors share a stale timeline, only the first request triggers a reload; "
+ + "the rest are served from the already-synced view. Set to 0 to disable.")
+ public int staleClientTimelineCacheSize = 10;
Review Comment:
🤖 non-blocking: `EmbeddedTimelineService.startServer` doesn't pass this value in from `HoodieWriteConfig`, so on the embedded server, where the production report came from, the cache always runs at size 10 and users can't set it to 0 if it misbehaves; would it make sense to wire it through the way the marker batch configs are?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>
##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -764,22 +777,121 @@ private boolean isLocalViewBehind(Context ctx) {
*/
private boolean syncIfLocalViewBehind(Context ctx) {
String basePath = ctx.queryParam(RemoteHoodieTableFileSystemView.BASEPATH_PARAM);
+ String timelineHashFromClient = getTimelineHashParam(ctx);
SyncableFileSystemView view = viewManager.getFileSystemView(basePath);
synchronized (view) {
+ HoodieTimeline localTimeline = view.getTimeline();
+ // The final check already accepts a trailing clean with an otherwise matching timeline, so a
+ // reload would be discarded work. Checked before isLocalViewBehind, which hashes the whole
+ // timeline, because this only inspects the last instant.
+ // Other extensions still need a sync: restore may have deleted their extra instants.
+ if (!shouldThrowExceptionIfLocalViewBehind(localTimeline, timelineHashFromClient)) {
+ return false;
+ }
+ // A reload already performed for this exact client timeline cannot produce a different
+ // outcome while the server timeline is unchanged, so skip it for the remaining stale clients.
+ if (hasAlreadySyncedFor(basePath, localTimeline, timelineHashFromClient, getLastInstantTsParam(ctx))) {
+ return false;
+ }
if (isLocalViewBehind(ctx)) {
String lastKnownInstantFromClient = getLastInstantTsParam(ctx);
- HoodieTimeline localTimeline = viewManager.getFileSystemView(basePath).getTimeline();
if (log.isInfoEnabled()) {
log.info("Syncing view as client passed last known instant {} as last known instant but server has the following last instant on timeline: {}",
lastKnownInstantFromClient, localTimeline.lastInstant());
}
+ Option<HoodieInstant> lastInstantBeforeSync = localTimeline.lastInstant();
view.sync();
+ recordSyncedClientTimeline(basePath, view.getTimeline(), lastInstantBeforeSync, timelineHashFromClient,
+ lastKnownInstantFromClient);
return true;
}
}
return false;
}
+ /**
+ * Whether a reload has already been performed for this client timeline and can be skipped.
+ *
+ * <p>Only a previously observed reload is treated as evidence; the client hash itself is never
+ * interpreted as proof that the server is ahead. The client's last known instant is re-checked
+ * on every hit so that a restore, which deletes completed instants from storage and moves the
+ * server timeline backwards, drops the entry instead of suppressing a reload that is now needed.
+ */
+ private boolean hasAlreadySyncedFor(String basePath, HoodieTimeline localTimeline,
+ String timelineHashFromClient, String lastKnownInstantFromClient) {
+ if (!isCacheableClientTimeline(lastKnownInstantFromClient)) {
+ return false;
+ }
+ Map<String, String> syncedHashes = syncedClientTimelineHashes.get(basePath);
+ if (syncedHashes == null) {
+ return false;
+ }
+ String serverHashWhenSynced = syncedHashes.get(cacheKey(timelineHashFromClient, lastKnownInstantFromClient));
+ // Only the reload observed against this exact server timeline is evidence. Once the server
+ // timeline moves, for any reason, a reload may produce a different result and must run again.
+ if (serverHashWhenSynced == null || !serverHashWhenSynced.equals(localTimeline.getTimelineHash())) {
+ return false;
+ }
+ if (!localTimeline.containsOrBeforeTimelineStarts(lastKnownInstantFromClient)) {
+ syncedHashes.remove(cacheKey(timelineHashFromClient, lastKnownInstantFromClient));
+ return false;
+ }
+ return true;
+ }
+
+ /**
+ * The refresh decision depends on the client's last known instant as well as its timeline hash,
+ * so both identify a recorded reload.
+ */
+ private String cacheKey(String timelineHashFromClient, String lastKnownInstantFromClient) {
+ return timelineHashFromClient + "_" + lastKnownInstantFromClient;
+ }
+
+ /**
+ * Whether a reload for this client is worth recording.
+ *
+ * <p>A client that sent no last known instant has not said where it stands, which only happens
+ * before its timeline holds a completed instant. There are no file slices to serve from a stale
+ * view at that point, so nothing is recorded rather than treating the absent value as an
+ * identity shared by unrelated clients.
+ */
+ private boolean isCacheableClientTimeline(String lastKnownInstantFromClient) {
+ return timelineServiceConfig.staleClientTimelineCacheSize > 0
+ && !HoodieTimeline.INVALID_INSTANT_TS.equals(lastKnownInstantFromClient)
+ && !StringUtils.isNullOrEmpty(lastKnownInstantFromClient);
+ }
+
+ /**
+ * Records that a reload was performed for this client timeline, so that the remaining stale
+ * clients sharing it do not each trigger another one.
+ *
+ * <p>Nothing is recorded when the reload dropped instants, which indicates the server timeline
+ * moved backwards rather than forwards and the previous records can no longer be trusted.
+ */
+ private void recordSyncedClientTimeline(String basePath, HoodieTimeline syncedTimeline,
+ Option<HoodieInstant> lastInstantBeforeSync, String timelineHashFromClient,
+ String lastKnownInstantFromClient) {
+ int cacheSize = timelineServiceConfig.staleClientTimelineCacheSize;
+ if (!isCacheableClientTimeline(lastKnownInstantFromClient)) {
+ return;
+ }
+ if (lastInstantBeforeSync.isPresent()
+ && !syncedTimeline.containsInstant(lastInstantBeforeSync.get().requestedTime())) {
+ // Instants were deleted from storage, e.g. by a restore. Earlier records described a
+ // timeline that no longer exists, so discard them rather than reason about what survived.
+ syncedClientTimelineHashes.remove(basePath);
+ return;
+ }
+ syncedClientTimelineHashes
+ .computeIfAbsent(basePath, ignored -> new LinkedHashMap<String, String>(cacheSize + 1, 0.75f, false) {
Review Comment:
🤖 non-blocking: `refreshTable` and `unregisterBasePath` replace the view object through `clearFileSystemView`, so a request still inside `synchronized (oldView)` and a new one inside `synchronized (newView)` can both `put` into this `LinkedHashMap`; could the inner map be wrapped in `Collections.synchronizedMap(...)` (or locked on directly) so it doesn't rely on the view monitor?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>
##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -764,22 +777,121 @@ private boolean isLocalViewBehind(Context ctx) {
*/
private boolean syncIfLocalViewBehind(Context ctx) {
String basePath = ctx.queryParam(RemoteHoodieTableFileSystemView.BASEPATH_PARAM);
+ String timelineHashFromClient = getTimelineHashParam(ctx);
SyncableFileSystemView view = viewManager.getFileSystemView(basePath);
synchronized (view) {
+ HoodieTimeline localTimeline = view.getTimeline();
+ // The final check already accepts a trailing clean with an otherwise matching timeline, so a
+ // reload would be discarded work. Checked before isLocalViewBehind, which hashes the whole
+ // timeline, because this only inspects the last instant.
+ // Other extensions still need a sync: restore may have deleted their extra instants.
+ if (!shouldThrowExceptionIfLocalViewBehind(localTimeline, timelineHashFromClient)) {
+ return false;
+ }
+ // A reload already performed for this exact client timeline cannot produce a different
+ // outcome while the server timeline is unchanged, so skip it for the remaining stale clients.
+ if (hasAlreadySyncedFor(basePath, localTimeline, timelineHashFromClient, getLastInstantTsParam(ctx))) {
+ return false;
+ }
if (isLocalViewBehind(ctx)) {
String lastKnownInstantFromClient = getLastInstantTsParam(ctx);
- HoodieTimeline localTimeline = viewManager.getFileSystemView(basePath).getTimeline();
if (log.isInfoEnabled()) {
log.info("Syncing view as client passed last known instant {} as last known instant but server has the following last instant on timeline: {}",
lastKnownInstantFromClient, localTimeline.lastInstant());
}
+ Option<HoodieInstant> lastInstantBeforeSync = localTimeline.lastInstant();
view.sync();
+ recordSyncedClientTimeline(basePath, view.getTimeline(), lastInstantBeforeSync, timelineHashFromClient,
+ lastKnownInstantFromClient);
return true;
}
}
return false;
}
+ /**
+ * Whether a reload has already been performed for this client timeline and can be skipped.
+ *
+ * <p>Only a previously observed reload is treated as evidence; the client hash itself is never
+ * interpreted as proof that the server is ahead. The client's last known instant is re-checked
+ * on every hit so that a restore, which deletes completed instants from storage and moves the
+ * server timeline backwards, drops the entry instead of suppressing a reload that is now needed.
+ */
+ private boolean hasAlreadySyncedFor(String basePath, HoodieTimeline localTimeline,
+ String timelineHashFromClient, String lastKnownInstantFromClient) {
+ if (!isCacheableClientTimeline(lastKnownInstantFromClient)) {
+ return false;
+ }
+ Map<String, String> syncedHashes = syncedClientTimelineHashes.get(basePath);
+ if (syncedHashes == null) {
+ return false;
+ }
+ String serverHashWhenSynced = syncedHashes.get(cacheKey(timelineHashFromClient, lastKnownInstantFromClient));
+ // Only the reload observed against this exact server timeline is evidence. Once the server
+ // timeline moves, for any reason, a reload may produce a different result and must run again.
+ if (serverHashWhenSynced == null || !serverHashWhenSynced.equals(localTimeline.getTimelineHash())) {
+ return false;
+ }
+ if (!localTimeline.containsOrBeforeTimelineStarts(lastKnownInstantFromClient)) {
Review Comment:
🤖 non-blocking: this re-check only runs while the server timeline still hashes to the recorded post-sync value, so it can't see a restore that reached storage after that reload and really only covers a reload that never reached the client's instant; could the javadoc above say so, since restore is actually handled by the hash comparison and the clear in `recordSyncedClientTimeline`?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>
##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -764,22 +777,121 @@ private boolean isLocalViewBehind(Context ctx) {
*/
private boolean syncIfLocalViewBehind(Context ctx) {
String basePath = ctx.queryParam(RemoteHoodieTableFileSystemView.BASEPATH_PARAM);
+ String timelineHashFromClient = getTimelineHashParam(ctx);
SyncableFileSystemView view = viewManager.getFileSystemView(basePath);
synchronized (view) {
+ HoodieTimeline localTimeline = view.getTimeline();
+ // The final check already accepts a trailing clean with an otherwise matching timeline, so a
+ // reload would be discarded work. Checked before isLocalViewBehind, which hashes the whole
+ // timeline, because this only inspects the last instant.
+ // Other extensions still need a sync: restore may have deleted their extra instants.
+ if (!shouldThrowExceptionIfLocalViewBehind(localTimeline, timelineHashFromClient)) {
+ return false;
+ }
+ // A reload already performed for this exact client timeline cannot produce a different
+ // outcome while the server timeline is unchanged, so skip it for the remaining stale clients.
+ if (hasAlreadySyncedFor(basePath, localTimeline, timelineHashFromClient, getLastInstantTsParam(ctx))) {
+ return false;
+ }
if (isLocalViewBehind(ctx)) {
String lastKnownInstantFromClient = getLastInstantTsParam(ctx);
- HoodieTimeline localTimeline = viewManager.getFileSystemView(basePath).getTimeline();
if (log.isInfoEnabled()) {
log.info("Syncing view as client passed last known instant {} as last known instant but server has the following last instant on timeline: {}",
lastKnownInstantFromClient, localTimeline.lastInstant());
}
+ Option<HoodieInstant> lastInstantBeforeSync = localTimeline.lastInstant();
view.sync();
+ recordSyncedClientTimeline(basePath, view.getTimeline(), lastInstantBeforeSync, timelineHashFromClient,
+ lastKnownInstantFromClient);
return true;
}
}
return false;
}
+ /**
+ * Whether a reload has already been performed for this client timeline and can be skipped.
+ *
+ * <p>Only a previously observed reload is treated as evidence; the client hash itself is never
+ * interpreted as proof that the server is ahead. The client's last known instant is re-checked
+ * on every hit so that a restore, which deletes completed instants from storage and moves the
+ * server timeline backwards, drops the entry instead of suppressing a reload that is now needed.
+ */
+ private boolean hasAlreadySyncedFor(String basePath, HoodieTimeline localTimeline,
+ String timelineHashFromClient, String lastKnownInstantFromClient) {
+ if (!isCacheableClientTimeline(lastKnownInstantFromClient)) {
+ return false;
+ }
+ Map<String, String> syncedHashes = syncedClientTimelineHashes.get(basePath);
+ if (syncedHashes == null) {
+ return false;
+ }
+ String serverHashWhenSynced = syncedHashes.get(cacheKey(timelineHashFromClient, lastKnownInstantFromClient));
+ // Only the reload observed against this exact server timeline is evidence. Once the server
+ // timeline moves, for any reason, a reload may produce a different result and must run again.
+ if (serverHashWhenSynced == null || !serverHashWhenSynced.equals(localTimeline.getTimelineHash())) {
+ return false;
+ }
+ if (!localTimeline.containsOrBeforeTimelineStarts(lastKnownInstantFromClient)) {
+ syncedHashes.remove(cacheKey(timelineHashFromClient, lastKnownInstantFromClient));
+ return false;
+ }
+ return true;
+ }
+
+ /**
+ * The refresh decision depends on the client's last known instant as well as its timeline hash,
+ * so both identify a recorded reload.
+ */
+ private String cacheKey(String timelineHashFromClient, String lastKnownInstantFromClient) {
+ return timelineHashFromClient + "_" + lastKnownInstantFromClient;
+ }
+
+ /**
+ * Whether a reload for this client is worth recording.
+ *
+ * <p>A client that sent no last known instant has not said where it stands, which only happens
+ * before its timeline holds a completed instant. There are no file slices to serve from a stale
+ * view at that point, so nothing is recorded rather than treating the absent value as an
+ * identity shared by unrelated clients.
+ */
+ private boolean isCacheableClientTimeline(String lastKnownInstantFromClient) {
+ return timelineServiceConfig.staleClientTimelineCacheSize > 0
+ && !HoodieTimeline.INVALID_INSTANT_TS.equals(lastKnownInstantFromClient)
+ && !StringUtils.isNullOrEmpty(lastKnownInstantFromClient);
+ }
+
+ /**
+ * Records that a reload was performed for this client timeline, so that the remaining stale
+ * clients sharing it do not each trigger another one.
+ *
+ * <p>Nothing is recorded when the reload dropped instants, which indicates the server timeline
+ * moved backwards rather than forwards and the previous records can no longer be trusted.
+ */
+ private void recordSyncedClientTimeline(String basePath, HoodieTimeline syncedTimeline,
+ Option<HoodieInstant> lastInstantBeforeSync, String timelineHashFromClient,
+ String lastKnownInstantFromClient) {
+ int cacheSize = timelineServiceConfig.staleClientTimelineCacheSize;
+ if (!isCacheableClientTimeline(lastKnownInstantFromClient)) {
+ return;
Review Comment:
🤖 nit: could you move the LRU `LinkedHashMap` construction into a small named helper (e.g. `newBoundedSyncedHashes(cacheSize)`)? The anonymous subclass inside `computeIfAbsent` makes `recordSyncedClientTimeline` hard to scan.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>
##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -764,22 +777,121 @@ private boolean isLocalViewBehind(Context ctx) {
*/
private boolean syncIfLocalViewBehind(Context ctx) {
String basePath = ctx.queryParam(RemoteHoodieTableFileSystemView.BASEPATH_PARAM);
+ String timelineHashFromClient = getTimelineHashParam(ctx);
SyncableFileSystemView view = viewManager.getFileSystemView(basePath);
synchronized (view) {
+ HoodieTimeline localTimeline = view.getTimeline();
+ // The final check already accepts a trailing clean with an otherwise matching timeline, so a
+ // reload would be discarded work. Checked before isLocalViewBehind, which hashes the whole
+ // timeline, because this only inspects the last instant.
+ // Other extensions still need a sync: restore may have deleted their extra instants.
+ if (!shouldThrowExceptionIfLocalViewBehind(localTimeline, timelineHashFromClient)) {
+ return false;
+ }
+ // A reload already performed for this exact client timeline cannot produce a different
+ // outcome while the server timeline is unchanged, so skip it for the remaining stale clients.
+ if (hasAlreadySyncedFor(basePath, localTimeline, timelineHashFromClient, getLastInstantTsParam(ctx))) {
+ return false;
+ }
if (isLocalViewBehind(ctx)) {
String lastKnownInstantFromClient = getLastInstantTsParam(ctx);
- HoodieTimeline localTimeline = viewManager.getFileSystemView(basePath).getTimeline();
if (log.isInfoEnabled()) {
log.info("Syncing view as client passed last known instant {} as last known instant but server has the following last instant on timeline: {}",
lastKnownInstantFromClient, localTimeline.lastInstant());
}
+ Option<HoodieInstant> lastInstantBeforeSync = localTimeline.lastInstant();
view.sync();
+ recordSyncedClientTimeline(basePath, view.getTimeline(), lastInstantBeforeSync, timelineHashFromClient,
+ lastKnownInstantFromClient);
return true;
}
}
return false;
}
+ /**
+ * Whether a reload has already been performed for this client timeline and can be skipped.
+ *
+ * <p>Only a previously observed reload is treated as evidence; the client hash itself is never
+ * interpreted as proof that the server is ahead. The client's last known instant is re-checked
+ * on every hit so that a restore, which deletes completed instants from storage and moves the
+ * server timeline backwards, drops the entry instead of suppressing a reload that is now needed.
+ */
+ private boolean hasAlreadySyncedFor(String basePath, HoodieTimeline localTimeline,
+ String timelineHashFromClient, String lastKnownInstantFromClient) {
+ if (!isCacheableClientTimeline(lastKnownInstantFromClient)) {
+ return false;
+ }
+ Map<String, String> syncedHashes = syncedClientTimelineHashes.get(basePath);
+ if (syncedHashes == null) {
+ return false;
+ }
+ String serverHashWhenSynced = syncedHashes.get(cacheKey(timelineHashFromClient, lastKnownInstantFromClient));
+ // Only the reload observed against this exact server timeline is evidence. Once the server
+ // timeline moves, for any reason, a reload may produce a different result and must run again.
+ if (serverHashWhenSynced == null || !serverHashWhenSynced.equals(localTimeline.getTimelineHash())) {
+ return false;
+ }
+ if (!localTimeline.containsOrBeforeTimelineStarts(lastKnownInstantFromClient)) {
+ syncedHashes.remove(cacheKey(timelineHashFromClient, lastKnownInstantFromClient));
+ return false;
+ }
+ return true;
+ }
+
+ /**
+ * The refresh decision depends on the client's last known instant as well as its timeline hash,
+ * so both identify a recorded reload.
+ */
+ private String cacheKey(String timelineHashFromClient, String lastKnownInstantFromClient) {
+ return timelineHashFromClient + "_" + lastKnownInstantFromClient;
+ }
+
+ /**
+ * Whether a reload for this client is worth recording.
+ *
+ * <p>A client that sent no last known instant has not said where it stands, which only happens
+ * before its timeline holds a completed instant. There are no file slices to serve from a stale
+ * view at that point, so nothing is recorded rather than treating the absent value as an
+ * identity shared by unrelated clients.
Review Comment:
🤖 nit: `isCacheableClientTimeline` also checks `staleClientTimelineCacheSize > 0`, so the name reads as only about the client timeline; would `isCachingEnabledFor(lastKnownInstantFromClient)` or splitting out the config check be clearer?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.</i></sub>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] perf(timeline-service): skip reloads already performed for a stale client timeline [hudi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20239:
URL: https://github.com/apache/hudi/pull/20239#issuecomment-6070890242
## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/20239?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `50.00000%` with `20 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 79.82%. Comparing base ([`60caf5f`](https://app.codecov.io/gh/apache/hudi/commit/60caf5f423989ff6d07136407bc8b0c20ee04c65?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`6cade52`](https://app.codecov.io/gh/apache/hudi/commit/6cade52192884ccdc17dfc2c5be41cd8517b2bbb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 18 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/20239?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...g/apache/hudi/timeline/service/RequestHandler.java](https://app.codecov.io/gh/apache/hudi/pull/20239?src=pr&el=tree&filepath=hudi-timeline-service%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Ftimeline%2Fservice%2FRequestHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS10aW1lbGluZS1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL3RpbWVsaW5lL3NlcnZpY2UvUmVxdWVzdEhhbmRsZXIuamF2YQ==) | 50.00% | [10 Missing and 10 partials :warning: ](https://app.codecov.io/gh/apache/hudi/pull/20239?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #20239 +/- ##
============================================
- Coverage 80.49% 79.82% -0.67%
- Complexity 34953 35343 +390
============================================
Files 2549 2551 +2
Lines 143060 145951 +2891
Branches 17392 17984 +592
============================================
+ Hits 115151 116508 +1357
- Misses 20004 21384 +1380
- Partials 7905 8059 +154
```
| [Components](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.65% <ø> (-2.35%)` | :arrow_down: |
| [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.05% <ø> (-0.49%)` | :arrow_down: |
| [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.96% <ø> (ø)` | |
| [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.85% <ø> (ø)` | |
| [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.18% <ø> (ø)` | |
| [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.78% <ø> (ø)` | |
| [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.84% <ø> (-0.20%)` | :arrow_down: |
| [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.23% <ø> (ø)` | |
| [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.80% <ø> (+0.30%)` | :arrow_up: |
| [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.22% <50.00%> (-1.08%)` | :arrow_down: |
| [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.00% <ø> (ø)` | |
| [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/20239/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.20% <ø> (ø)` | |
| [Flag](https://app.codecov.io/gh/apache/hudi/pull/20239/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/20239/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.99% <50.00%> (+0.02%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/20239?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [.../apache/hudi/timeline/service/TimelineService.java](https://app.codecov.io/gh/apache/hudi/pull/20239?src=pr&el=tree&filepath=hudi-timeline-service%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Ftimeline%2Fservice%2FTimelineService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS10aW1lbGluZS1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL3RpbWVsaW5lL3NlcnZpY2UvVGltZWxpbmVTZXJ2aWNlLmphdmE=) | `51.37% <ø> (ø)` | |
| [...g/apache/hudi/timeline/service/RequestHandler.java](https://app.codecov.io/gh/apache/hudi/pull/20239?src=pr&el=tree&filepath=hudi-timeline-service%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fhudi%2Ftimeline%2Fservice%2FRequestHandler.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS10aW1lbGluZS1zZXJ2aWNlL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9odWRpL3RpbWVsaW5lL3NlcnZpY2UvUmVxdWVzdEhhbmRsZXIuamF2YQ==) | `87.09% <50.00%> (-3.14%)` | :arrow_down: |
... and [36 files with indirect coverage changes](https://app.codecov.io/gh/apache/hudi/pull/20239/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org