You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "prashantwason (via GitHub)" <gi...@apache.org> on 2026/10/08 00:13:13 UTC

[PR] fix(timeline-service): sync the view only once per stale client timeline [hudi]

prashantwason opened a new pull request, #20221:
URL: https://github.com/apache/hudi/pull/20221

   ### Describe the issue this Pull Request addresses
   
   The timeline server re-syncs its file system view on **every** request when an async clean completes while a write is in progress. Under a large writer this serializes all requests behind full timeline reloads and makes executors fail with `NoHttpResponseException`.
   
   **Scenario (seen in production on a COPY_ON_WRITE upsert with ~1000 executors):**
   
   1. Writer starts commit `C1` and builds its timeline (last completed instant `C0`, timeline hash `H_old`). This timeline is shipped to all executors; every executor request to the timeline server carries `lastinstantts=C0, timelinehash=H_old`.
   2. An async clean `CL` (e.g. scheduled by a table service manager) completes while `C1` is being written.
   3. A request carrying a newer client timeline makes the timeline server sync its view. The server's timeline now contains `CL`.
   4. From then on, for every executor request `isLocalViewBehind` compares `H_old` with the server's hash. They never match again because the server has `CL` and the client does not. So **every request** calls `view.sync()`, which reloads the active timeline from storage, while holding `synchronized (view)`. Syncing again cannot make the hashes match.
   5. Hundreds of concurrent requests queue behind that lock and the server's request threads are exhausted. Marker creation (`TimelineServerBasedWriteMarkers.create`) hangs for ~10 minutes and fails:
   
   ```
   HoodieRemoteException: Failed to create marker file <partition>/<fileId>_..._<C1>.parquet.marker.MERGE
   <driver-host>:<port> failed to respond
   Caused by: org.apache.http.NoHttpResponseException: <driver-host>:<port> failed to respond
   ```
   
   The driver log shows the same line for every request:
   
   ```
   Syncing view as client passed last known instant C0 as last known instant but server has the following last instant on timeline: Option{val=[CL__clean__COMPLETED]}
   ```
   
   Tasks exhaust their retries and the write stage aborts. The post-request check `shouldThrowExceptionIfLocalViewBehind` already tolerates this exact case (a trailing completed clean), so the requests themselves would succeed. Only the repeated syncs are the problem.
   
   ### Summary and Changelog
   
   For a given stale client timeline, the timeline server now syncs its view once instead of once per request.
   
   A client's timeline hash describes a timeline it loaded from storage before sending the request. Once the server has synced its view after receiving a request carrying hash `H`, the server's view is at least as recent as any client still holding `H`. Further syncs for `H` are redundant, so the server remembers `H` and skips them.
   
   - `RequestHandler`: added `syncedClientTimelineHashes`, which maps each file system view to the set of client timeline hashes that view has already been synced past.
     - The map is keyed by view instance with weak keys. A view that is cleared and re-created starts empty, so its first sync is never skipped, and entries go away with dropped views. The map is synchronized; each per-view set is only touched while holding that view's lock, which `syncIfLocalViewBehind` already takes.
     - Each per-view set is bounded (`MAX_SYNCED_CLIENT_TIMELINE_HASHES = 32`) and backed by an insertion-ordered `LinkedHashMap` that evicts the oldest entry (FIFO). Evicting a hash that is still in use only costs one extra sync for it; correctness is unaffected.
     - `syncIfLocalViewBehind` skips the sync when the client's hash is in the view's set, and records the hash after a sync. Requests without a timeline hash are never skipped.
   - Added a `VIEW_SYNC` counter to the `TimelineService` metrics registry, so redundant syncs are observable.
   - `TestRequestHandler#testViewSyncedOnceForStaleClientTimeline` reproduces the scenario: commit, stale client timeline, a clean completes, the server syncs for the new timeline, then 32 concurrent requests with the stale hash. It asserts exactly one sync. Without the fix the test fails with `expected: <1> but was: <32>`.
   
   ### Impact
   
   Performance and stability of the timeline server only. There are no API, config, or storage format changes. Writers whose executors carry a timeline that predates a concurrently completed clean (or any other instant) no longer trigger a full timeline reload per request.
   
   ### Risk Level
   
   low
   
   A sync is skipped only when the same view has already been synced after a request carrying the exact same client timeline hash was received, which guarantees the view is at least as recent as that client. New views, new hashes, and requests without a hash behave as before. The post-request check (`shouldThrowExceptionIfLocalViewBehind`) is unchanged. All `hudi-timeline-service` tests pass, including `TestRemoteHoodieTableFileSystemView`.
   
   ### Documentation Update
   
   none
   
   ### 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] fix(timeline-service): sync the view only once per stale client timeline [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #20221:
URL: https://github.com/apache/hudi/pull/20221#discussion_r4213955444


##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -105,6 +113,29 @@ public class RequestHandler {
   private final Registry metricsRegistry = Registry.getRegistry("TimelineService");
   private final ScheduledExecutorService asyncResultService;
 
+  /**
+   * Client timeline hashes for which the local file system view has already been synced, per view.
+   *
+   * <p>A client's timeline hash describes a timeline the client loaded from storage before it sent the request.
+   * Once the server has synced its view after receiving a request carrying hash {@code H}, the server's view
+   * is at least as recent as any client still holding {@code H}. Further requests carrying {@code H} can still
+   * fail the hash comparison in {@code isLocalViewBehind} (e.g. a clean completed after the client loaded its
+   * timeline, so the server timeline has an extra instant the client does not know about), but syncing again
+   * cannot make the server any less behind. Remembering {@code H} lets us skip those redundant syncs.
+   *
+   * <p>Without this, every such request does a full timeline reload from storage under the view lock. With
+   * hundreds of executors issuing view and marker requests, the requests serialize behind that lock, the
+   * server's request threads are exhausted and clients time out with {@code NoHttpResponseException}.
+   *
+   * <p>The map is keyed by the view instance (identity) rather than the base path, so that when a view is
+   * cleared and re-created the new view starts with no remembered hashes and its first sync is never skipped.
+   * Weak keys let entries go away together with views dropped by the {@link FileSystemViewManager}.
+   * The map itself is shared across views and is therefore synchronized; each per-view set is only accessed
+   * while holding the lock on its view.

Review Comment:
   A comparison of these two from AI:
   
   <html><head></head><body><p><strong><span>I prefer #19946’s approach of checking the current timeline relationship, but I would narrow it to the existing trailing-clean exception. I wouldn’t merge either current implementation unchanged.</span></strong></p><p><span>I compared #19946 at </span><code dir="ltr">fe2dfa3</code><span> and #20221 at </span><code dir="ltr">af8e0a0</code><span>.</span></p><div><div><div>
     | #19946: compare timeline prefix | #20221: remember client hashes
   -- | -- | --
   Reason to skip sync | Server’s current timeline contains the exact client prefix | This view previously synced after receiving that hash
   Original trailing-clean scenario | Zero redundant syncs | One sync, then skips while cached
   Additional state | None | Per-view cache, eviction, lifecycle handling
   Main concern | An extra server instant might have been deleted from storage | A previous sync does not prove a later sync is unnecessary
   
   </div></div></div><p><span>The decisive issue is that </span><strong><span>Hudi’s timeline does not only grow</span></strong><span>. Restore and rollback can remove instants. Therefore, my earlier explanation needs this qualification: syncing cannot fix the mismatch </span><strong><span>while storage still contains the extra instant</span></strong><span>.</span></p><p><span>Consider this state after a restore removes commit </span><code dir="ltr">002</code><span>:</span></p><pre dir="ltr"><code>Client:           [001]
   Current storage:  [001]
   Cached server:    [001, 002]</code></pre><p><span>Here, </span><strong><span>syncing would make the hashes match</span></strong><span>.</span></p><p><span>#19946 sees an exact prefix and skips that necessary sync. Its unchanged final check then rejects the request. A </span><a href="https://github.com/apache/hudi/pull/19946#discussion_r4056082803"><span><span><span>review comment reports this restore failure in tests</span></span></span></a><span>; the current </span><a href="https://github.com/apache/hudi/blob/fe2dfa38db6c97e954661c2dc5bd11200a47f051/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java#L770-L795"><span><span><span>prefix check</span></span></span></a><span> supports that diagnosis. I haven’t independently rerun those failures.</span></p><p><span>#20221 handles that situation if the client hash is new to its cache. But </span><strong><span>if that hash was recorded earlier</span></strong><span>, it skips the sync too. Its cach
 e is tied to the view object, rather than the current server timeline, and records the hash immediately after </span><code dir="ltr">sync()</code><span>, before final validation. Thus “we synced for this hash once” can remain cached across subsequent timeline changes. This is a code-derived concern, not a reproduced failure. </span><a href="https://github.com/apache/hudi/blob/af8e0a0fa85510784d856034dac7bdb5b1793fae/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java#L823-L847"><span><span><span>Cache implementation</span></span></span></a><span>.</span></p><p><strong><span>My recommendation is a smaller, stateless fix:</span></strong></p><ul><li><span>Keep the final consistency check unchanged.</span></li><li><span>Before syncing, recognize the exact mismatch already accepted by that check: the server has a trailing completed clean, and excluding that clean produces the client’s hash.</span></li><li><span>Skip sync for that case; retain th
 e existing sync behavior for other mismatches.</span></li><li><span>Add restore/deleted-instant coverage alongside the concurrent stale-client regression.</span></li></ul><p><span>That directly fixes the reported clean-induced sync storm, avoids cache invalidation concerns, and preserves recovery when extra commit instants disappear. The </span><a href="https://github.com/apache/hudi/blob/fe2dfa38db6c97e954661c2dc5bd11200a47f051/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java#L821-L826"><span><span><span>existing trailing-clean check</span></span></span></a><span> provides the starting point.</span></p><p><span>One review detail: </span><strong><span>#19946’s description is stale.</span></strong><span> It says the prefix decision also relaxes final validation, but its current code separates refresh from final validation and preserves rejection/fallback. I would judge the current code, and update that description before merging.</span></p></
 body></html>**I prefer #19946’s approach of checking the current timeline relationship, but I would narrow it to the existing trailing-clean exception. I wouldn’t merge either current implementation unchanged.**
   
   I compared #19946 at `fe2dfa3` and #20221 at `af8e0a0`.
   
   | | #19946: compare timeline prefix | #20221: remember client hashes |
   |---|---|---|
   | Reason to skip sync | Server’s current timeline contains the exact client prefix | This view previously synced after receiving that hash |
   | Original trailing-clean scenario | Zero redundant syncs | One sync, then skips while cached |
   | Additional state | None | Per-view cache, eviction, lifecycle handling |
   | Main concern | An extra server instant might have been deleted from storage | A previous sync does not prove a later sync is unnecessary |
   
   The decisive issue is that **Hudi’s timeline does not only grow**. Restore and rollback can remove instants. Therefore, my earlier explanation needs this qualification: syncing cannot fix the mismatch **while storage still contains the extra instant**.
   
   Consider this state after a restore removes commit `002`:
   
   ```text
   Client:           [001]
   Current storage:  [001]
   Cached server:    [001, 002]
   ```
   
   Here, **syncing would make the hashes match**.
   
   #19946 sees an exact prefix and skips that necessary sync. Its unchanged final check then rejects the request. A [[review comment reports this restore failure in tests](https://github.com/apache/hudi/pull/19946#discussion_r4056082803)](https://github.com/apache/hudi/pull/19946#discussion_r4056082803); the current [[prefix check](https://github.com/apache/hudi/blob/fe2dfa38db6c97e954661c2dc5bd11200a47f051/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java#L770-L795)](https://github.com/apache/hudi/blob/fe2dfa38db6c97e954661c2dc5bd11200a47f051/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java#L770-L795) supports that diagnosis. I haven’t independently rerun those failures.
   
   #20221 handles that situation if the client hash is new to its cache. But **if that hash was recorded earlier**, it skips the sync too. Its cache is tied to the view object, rather than the current server timeline, and records the hash immediately after `sync()`, before final validation. Thus “we synced for this hash once” can remain cached across subsequent timeline changes. This is a code-derived concern, not a reproduced failure. [[Cache implementation](https://github.com/apache/hudi/blob/af8e0a0fa85510784d856034dac7bdb5b1793fae/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java#L823-L847)](https://github.com/apache/hudi/blob/af8e0a0fa85510784d856034dac7bdb5b1793fae/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java#L823-L847).
   
   **My recommendation is a smaller, stateless fix:**
   
   - Keep the final consistency check unchanged.
   - Before syncing, recognize the exact mismatch already accepted by that check: the server has a trailing completed clean, and excluding that clean produces the client’s hash.
   - Skip sync for that case; retain the existing sync behavior for other mismatches.
   - Add restore/deleted-instant coverage alongside the concurrent stale-client regression.
   
   That directly fixes the reported clean-induced sync storm, avoids cache invalidation concerns, and preserves recovery when extra commit instants disappear. The [[existing trailing-clean check](https://github.com/apache/hudi/blob/fe2dfa38db6c97e954661c2dc5bd11200a47f051/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java#L821-L826)](https://github.com/apache/hudi/blob/fe2dfa38db6c97e954661c2dc5bd11200a47f051/hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java#L821-L826) provides the starting point.
   
   One review detail: **#19946’s description is stale.** It says the prefix decision also relaxes final validation, but its current code separates refresh from final validation and preserves rejection/fallback. I would judge the current code, and update that description before merging.



-- 
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] fix(timeline-service): do not sync the view when it is ahead of the client only by cleans [hudi]

Posted by "prashantwason (via GitHub)" <gi...@apache.org>.
prashantwason commented on PR #20221:
URL: https://github.com/apache/hudi/pull/20221#issuecomment-6052990793

   Closing this in favor of #19946, which fixes the same issue (repeated `view.sync()` when an async clean completes during a write) and was opened earlier and has already gone through review. Sorry for the noise, I missed it when searching before filing.
   
   One gap I found while working on this, which I've raised on #19946: if **more than one** clean completes after the client loaded its timeline, the trailing-clean exception no longer applies. Every request then syncs and is rejected with 400. Details and a test are in my comment there.
   


-- 
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] fix(timeline-service): sync the view only once per stale client timeline [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on PR #20221:
URL: https://github.com/apache/hudi/pull/20221#issuecomment-6051146043

   @prashantwason revised https://github.com/apache/hudi/pull/19946 and it looks like a much simpler fix than this patch.


-- 
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] fix(timeline-service): do not sync the view when it is ahead of the client only by cleans [hudi]

Posted by "prashantwason (via GitHub)" <gi...@apache.org>.
prashantwason closed pull request #20221: fix(timeline-service): do not sync the view when it is ahead of the client only by cleans
URL: https://github.com/apache/hudi/pull/20221


-- 
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] fix(timeline-service): sync the view only once per stale client timeline [hudi]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20221:
URL: https://github.com/apache/hudi/pull/20221#issuecomment-6049628780

   ## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/20221?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 `69.23077%` with `4 lines` in your changes missing coverage. Please review.
   :white_check_mark: Project coverage is 80.61%. Comparing base ([`7f35e7e`](https://app.codecov.io/gh/apache/hudi/commit/7f35e7eedcc002a5a1dd5f68b530a0911dc74d90?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`af8e0a0`](https://app.codecov.io/gh/apache/hudi/commit/af8e0a0fa85510784d856034dac7bdb5b1793fae?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/20221?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/20221?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==) | 69.23% | [0 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/hudi/pull/20221?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   #20221      +/-   ##
   ============================================
   - Coverage     80.61%   80.61%   -0.01%     
   - Complexity    35042    35043       +1     
   ============================================
     Files          2552     2552              
     Lines        143353   143366      +13     
     Branches      17442    17446       +4     
   ============================================
   + Hits         115571   115580       +9     
     Misses        19867    19867              
   - Partials       7915     7919       +4     
   ```
   
   | [Components](https://app.codecov.io/gh/apache/hudi/pull/20221/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/20221/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `84.16% <ø> (ø)` | |
   | [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/20221/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.82% <ø> (ø)` | |
   | [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/20221/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.98% <ø> (ø)` | |
   | [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/20221/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `74.01% <ø> (ø)` | |
   | [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/20221/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.19% <ø> (ø)` | |
   | [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/20221/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/20221/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `71.08% <ø> (ø)` | |
   | [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/20221/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.25% <ø> (ø)` | |
   | [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/20221/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `81.52% <ø> (ø)` | |
   | [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/20221/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `82.91% <69.23%> (-0.15%)` | :arrow_down: |
   | [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/20221/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/20221/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/20221/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/20221/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.97% <69.23%> (+0.01%)` | :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/20221?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...g/apache/hudi/timeline/service/RequestHandler.java](https://app.codecov.io/gh/apache/hudi/pull/20221?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==) | `89.67% <69.23%> (-0.56%)` | :arrow_down: |
   </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

Re: [PR] fix(timeline-service): sync the view only once per stale client timeline [hudi]

Posted by "danny0405 (via GitHub)" <gi...@apache.org>.
danny0405 commented on code in PR #20221:
URL: https://github.com/apache/hudi/pull/20221#discussion_r4213665933


##########
hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/RequestHandler.java:
##########
@@ -105,6 +113,29 @@ public class RequestHandler {
   private final Registry metricsRegistry = Registry.getRegistry("TimelineService");
   private final ScheduledExecutorService asyncResultService;
 
+  /**
+   * Client timeline hashes for which the local file system view has already been synced, per view.
+   *
+   * <p>A client's timeline hash describes a timeline the client loaded from storage before it sent the request.
+   * Once the server has synced its view after receiving a request carrying hash {@code H}, the server's view
+   * is at least as recent as any client still holding {@code H}. Further requests carrying {@code H} can still
+   * fail the hash comparison in {@code isLocalViewBehind} (e.g. a clean completed after the client loaded its
+   * timeline, so the server timeline has an extra instant the client does not know about), but syncing again
+   * cannot make the server any less behind. Remembering {@code H} lets us skip those redundant syncs.
+   *
+   * <p>Without this, every such request does a full timeline reload from storage under the view lock. With
+   * hundreds of executors issuing view and marker requests, the requests serialize behind that lock, the
+   * server's request threads are exhausted and clients time out with {@code NoHttpResponseException}.
+   *
+   * <p>The map is keyed by the view instance (identity) rather than the base path, so that when a view is
+   * cleared and re-created the new view starts with no remembered hashes and its first sync is never skipped.
+   * Weak keys let entries go away together with views dropped by the {@link FileSystemViewManager}.
+   * The map itself is shared across views and is therefore synchronized; each per-view set is only accessed
+   * while holding the lock on its view.

Review Comment:
   a fix for the similar purposes: https://github.com/apache/hudi/pull/19946, the issue is real, we need to decide which fix is preferable.



-- 
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