danny0405 commented on code in PR #9871:
URL: https://github.com/apache/hudi/pull/9871#discussion_r1378742958
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/timeline/HoodieTimelineArchiver.java:
##########
@@ -270,13 +270,41 @@ private List<HoodieInstant> getCommitInstantsToArchive()
throws IOException {
// stop at first savepoint commit
return !firstSavepoint.isPresent() ||
compareTimestamps(s.getTimestamp(), LESSER_THAN,
firstSavepoint.get().getTimestamp());
}
- }).filter(s -> earliestInstantToRetain
- .map(instant -> compareTimestamps(s.getTimestamp(), LESSER_THAN,
instant.getTimestamp()))
+ })
+ .filter(s -> earliestInstantToRetain.map(
+ instant -> compareTimestamps(s.getTimestamp(), LESSER_THAN,
instant.getTimestamp())
+ // for the compaction c2 in metadata table triggered by commit
c1 in data table,
+ // c2.getTimestamp() > c1.getTimestamp() because: c2 happens
before c1 completes,
+ // and we are generating new instant time for c2 after c1 has
started. Effectively,
Review Comment:
Shouldn’t we fix line 208 ~ line 222 instead?
And this issue also exists for the master code, the compaction instant is
delta_commit + “001” which happens before delta_commit with a greater instant
time, but we still only compares the instant directly by using the compaction
instant time.
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/timeline/HoodieTimelineArchiver.java:
##########
@@ -270,13 +270,41 @@ private List<HoodieInstant> getCommitInstantsToArchive()
throws IOException {
// stop at first savepoint commit
return !firstSavepoint.isPresent() ||
compareTimestamps(s.getTimestamp(), LESSER_THAN,
firstSavepoint.get().getTimestamp());
}
- }).filter(s -> earliestInstantToRetain
- .map(instant -> compareTimestamps(s.getTimestamp(), LESSER_THAN,
instant.getTimestamp()))
+ })
+ .filter(s -> earliestInstantToRetain.map(
+ instant -> compareTimestamps(s.getTimestamp(), LESSER_THAN,
instant.getTimestamp())
+ // for the compaction c2 in metadata table triggered by commit
c1 in data table,
+ // c2.getTimestamp() > c1.getTimestamp() because: c2 happens
before c1 completes,
+ // and we are generating new instant time for c2 after c1 has
started. Effectively,
Review Comment:
Shouldn’t we fix line 208 ~ line 222 instead?
And this issue also exists for the master code, the compaction instant is
delta_commit + “001” which happens before delta_commit with a greater instant
time, but we still only compares the instant directly by using the compaction
instant time.
--
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]
