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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
