[HUDI-5569] Fixing TableFileSystemView to detect early failed commits #7738
Conversation
| connectTableAndReloadMetaClient(tablePath); | ||
| HoodieTableFileSystemView fsView = new HoodieTableFileSystemView(metaClient, | ||
| metaClient.getActiveTimeline().getCommitsTimeline().filterCompletedInstants(), | ||
| TimelineUtils.getFirstNotCompleted(metaClient.getActiveTimeline().getCommitsTimeline()), |
There was a problem hiding this comment.
Is there any possibility just pass around the metaClient.getActiveTimeline().getCommitsTimeline() and resolve the first not completed instant inside the HoodieTableFileSystemView constructor. This can avoid many unnecessary changes and the code is cleaner to maintain.
There was a problem hiding this comment.
yeah, thought about it. couple of reasons.
- at times we do send the fileGroup fully to timeline server on which case we are sending entire timeline (not just completed) and might take up more space.
- also, we really don't need entire timeline. we just need the first Active instant. and hence took this route. Also, most of FileSystemView code assume its completed timeline and hence didn't want to change that assumption.
nsivabalan
left a comment
nsivabalan
left a comment
There was a problem hiding this comment.
responded to comments
| } | ||
| } | ||
|
|
||
| public static Option<String> getFirstNotCompleted(HoodieTimeline timeline) { |
There was a problem hiding this comment.
we should make this method getFirstInstant(timeline).
essentially irrespective fo whether its complete or inflight, first instant in write timeline.
|
|
||
| public static HoodieFileGroup toFileGroup(FileGroupDTO dto, HoodieTableMetaClient metaClient) { | ||
| HoodieFileGroup fileGroup = | ||
| new HoodieFileGroup(dto.partition, dto.id, TimelineDTO.toTimeline(dto.timeline, metaClient)); |
There was a problem hiding this comment.
we might need to fix FileGroupDTO to hold the first instant as well.
| * Initialize the view. | ||
| */ | ||
| protected void init(HoodieTableMetaClient metaClient, HoodieTimeline visibleActiveTimeline) { | ||
| protected void init(HoodieTableMetaClient metaClient, HoodieTimeline visibleActiveTimeline,Option<String> firstNotCompleted) { |
There was a problem hiding this comment.
nit: firstActiveInstant
| connectTableAndReloadMetaClient(tablePath); | ||
| HoodieTableFileSystemView fsView = new HoodieTableFileSystemView(metaClient, | ||
| metaClient.getActiveTimeline().getCommitsTimeline().filterCompletedInstants(), | ||
| TimelineUtils.getFirstNotCompleted(metaClient.getActiveTimeline().getCommitsTimeline()), |
There was a problem hiding this comment.
yeah, thought about it. couple of reasons.
- at times we do send the fileGroup fully to timeline server on which case we are sending entire timeline (not just completed) and might take up more space.
- also, we really don't need entire timeline. we just need the first Active instant. and hence took this route. Also, most of FileSystemView code assume its completed timeline and hence didn't want to change that assumption.
| // To get here: | ||
| // 1. the timestamp must be <= the last commit | ||
| // 2. not in the completed timeline | ||
| // 3. the timestamp must be >= the first active instant |
There was a problem hiding this comment.
Yeah, the #isBeforeTimelineStarts is only valid when the timeline is the whole complete active timeline.
nsivabalan
added
priority:critical
…se it was filtering out cleaning instances
…System View correctly
and rebase
jonvex
force-pushed
the
first_commit_fsv
branch
from
5cf1313 to
f2e3a99
Compare
codope
left a comment
codope
left a comment
There was a problem hiding this comment.
@jonvex Can you please rebase and resolve conflicts?
@danny0405 @nsivabalan Is this a blocker for 0.13.1?
|
Closing this in favor of #8783 |
6 participants
Change Logs
When we have some failed commits in the timeline before first successful commit, FS based listing could return data from the failed commit. This is not an issue w/ single writer. Could only happen when multi-writers are enabled or when async table services are enabled along w/ deltastreamer.
For eg, if timeline is:
c1.inflight, c2.complete,c3.complete
when we query hudi, data files from c1 is also returned. Fixing it as part of this patch.
Impact
FS based listing will not return data from failed commit.
Risk level (write none, low medium or high below)
low
Documentation Update
N/A
Contributor's checklist