prashantwason commented on PR #19946:
URL: https://github.com/apache/hudi/pull/19946#issuecomment-6052990577
Thanks for this fix. We hit this in production: a COPY_ON_WRITE upsert with
~1000 executors, where an async clean completed mid-write. After that, every
executor request re-synced the view under the view lock, the timeline server
stopped responding, and marker creation failed with `NoHttpResponseException`
after ~10 min. The stage aborted after all task retries. The current version of
this PR fixes that case. I had opened #20221 for the same issue before finding
this PR, and I've closed it in favor of this one.
One gap worth considering: the exemption reuses
`shouldThrowExceptionIfLocalViewBehind`, which only covers **one** trailing
clean. If two cleans complete after the client loaded its timeline:
```
client: [c1] (hash H)
server: [c1, clean2, clean3]
```
the last instant is a clean, but the instants before it are `[c1, clean2]`,
whose hash is not H. So `syncIfLocalViewBehind` still syncs on every request,
and the final check then rejects with 400. Clients behind
`PriorityBasedFileSystemView` fall back to their local view, but that still
costs one full reload per executor, and marker requests have no fallback.
Long-running writes with frequent async cleaning (e.g. from a table service
manager) can realistically see more than one clean complete mid-write.
A small extension that keeps the current behavior otherwise: walk back over
all trailing cleans and compare the client hash against the timeline cut before
each one. That also covers a client that has already seen some of the trailing
cleans:
```java
private boolean isLocalTimelineAheadOnlyByCleans(HoodieTimeline
localTimeline, String timelineHashFromClient) {
List<HoodieInstant> instants = localTimeline.getInstants();
// walk back over the trailing cleans, newest first
for (int i = instants.size() - 1; i >= 0 &&
instants.get(i).getAction().equals(HoodieTimeline.CLEAN_ACTION); i--) {
if
(localTimeline.findInstantsBefore(instants.get(i).requestedTime()).getTimelineHash().equals(timelineHashFromClient))
{
return true;
}
}
return false;
}
```
`shouldThrowExceptionIfLocalViewBehind` could become
`!isLocalTimelineAheadOnlyByCleans(...)` (for a non-empty timeline), so the
sync decision and the final check keep using the same predicate, as they do in
this PR. Non-clean differences, such as commits removed by a restore, still
sync and fail the final check exactly as before. Only the "server has extra
cleans" case is widened.
A test that reproduces it (fails with 400 on master; the current head of
this PR uses the same single-clean predicate):
```java
HoodieTestTable testTable = HoodieTestTable.of(metaClient);
testTable.addCommit("001");
HoodieTimeline staleClientTimeline =
metaClient.reloadActiveTimeline().filterCompletedAndCompactionInstants();
requestLatestDataFiles(staleClientTimeline); // server loads its
view, no sync
testTable.addClean("002");
testTable.addClean("003");
requestLatestDataFiles(metaClient.reloadActiveTimeline().filterCompletedAndCompactionInstants());
// server syncs once
// 32 concurrent requests with the stale timeline: expect 0 syncs and no 400s
```
Happy to send this as a follow-up after this PR merges if you'd rather keep
this one narrow.
--
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]
