danny0405 commented on code in PR #20273:
URL: https://github.com/apache/hudi/pull/20273#discussion_r4272609569
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -91,9 +102,19 @@ public class CompletionTimeQueryViewV2 implements
CompletionTimeQueryView, Seria
public CompletionTimeQueryViewV2(HoodieTableMetaClient metaClient) {
this.metaClient = metaClient;
this.instantTimeToCompletionTimeMap = new ConcurrentHashMap<>();
- this.cursorInstant =
metaClient.getActiveTimeline().firstInstant().map(HoodieInstant::requestedTime).orElse("");
+ HoodieTimeline activeTimeline = metaClient.getActiveTimeline();
+ // Instants at or after the cursor are assumed to be on the active
timeline, so it must start at the first
+ // write instant that is not savepointed: an older non-write instant (e.g.
a rollback the archiver has not reached)
+ // or a savepointed commit left behind by archival would otherwise hide
the archived write instants after it.
+ // Instants before the cursor that are still active are loaded eagerly by
#load. Without such a write instant,
+ // nothing after the last active instant can be archived.
+ this.firstActiveWriteInstant =
activeTimeline.getTimelineOfActions(HoodieTimeline.WRITE_AND_SAVEPOINT_ACTIONS).getFirstNonSavepointCommit()
Review Comment:
[P2] Preserve the no-archive-I/O path for ranges covered by a contiguous
active timeline
Moving the cursor from the first active instant to the first non-savepointed
write can introduce archive I/O even with
`hoodie.archive.beyond.savepoint=false` and no archival holes. For example, let
all archived instants precede Oct 1, with an active completed clean on Oct 1
and the first active write on Oct 3. For a query starting Oct 3 at noon,
`getInstantTimes()` computes its one-day lookback as Oct 2 at noon. Previously
that was after the cursor, so no archive load occurred; now it is before the
cursor and calls `loadCompletionTimeIncrementally()` for an interval containing
no archived instants. A leading sequence of retained savepointed writes can
produce the same effect.
`ArchivedTimelineLoaderV2.getFilteredFiles()` reads the latest archive
version/manifest before discovering that no files overlap. This adds storage
I/O and manifest processing, even though no archive Parquet file needs to be
read. The cursor only amortizes this within one view;
`IncrementalQueryAnalyzer.analyze()` creates a new view each time.
Could we retain a safe fast path for contiguous V2 timelines while
preserving the legacy-upgrade/hole fix, and add a loader-call-count regression
test for this fully active range?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/versioning/v2/CompletionTimeQueryViewV2.java:
##########
@@ -102,14 +123,18 @@ public CompletionTimeQueryViewV2(HoodieTableMetaClient
metaClient) {
*/
public boolean isCompleted(String instantTime) {
// archival does not proceed beyond the first savepoint, so any instant
before that is completed.
- return this.instantTimeToCompletionTimeMap.containsKey(instantTime) ||
isArchived(instantTime);
+ return this.instantTimeToCompletionTimeMap.containsKey(instantTime)
+ || InstantComparison.compareTimestamps(instantTime, LESSER_THAN,
this.firstNonSavepointCommit);
}
/**
* Returns whether the instant is archived.
*/
public boolean isArchived(String instantTime) {
- return InstantComparison.compareTimestamps(instantTime, LESSER_THAN,
this.firstNonSavepointCommit);
+ // with archival beyond savepoint, the write instants between a
savepointed commit and the first active write
+ // instant are archived as well, while the savepointed commit itself stays
active.
+ return InstantComparison.compareTimestamps(instantTime, LESSER_THAN,
this.firstActiveWriteInstant)
Review Comment:
[P2] Avoid archive lookup caused by an active clean/rollback before this
boundary
With archival beyond savepoint disabled, consider an entirely active
sequence `C1(savepointed), clean2, C3`. This new boundary is C3, so
`isArchived(clean2)` returns true even though clean2 is active. Previously the
write boundary was C1 and it returned false. The same applies to a rollback
between those writes.
This reaches the query path because `load()` stores completion times for all
completed actions, and the bounded `getInstantTimes()` path returns matching
map entries without restricting them to writes. If clean2 is in the
completion-time window, `splitInstantByActiveness()` now puts it in the
archived bucket and `analyze()` calls `getArchivedReadTimeline()`, even when
every selected write is active. Subsequent commit filtering removes the
non-write action, so the concern here is unnecessary archive access rather than
incorrect write rows. This can also happen independently of the cursor/lookback
issue when a delayed clean/rollback completes in a recent query window.
Could the analyzer exclude non-write entries from its archival decision (or
otherwise recognize these active actions), with a test asserting that an active
savepoint/clean/write sequence does not trigger an archived-timeline lookup?
--
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]
