yihua commented on code in PR #10992:
URL: https://github.com/apache/hudi/pull/10992#discussion_r1566585767
##########
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/metadata/HoodieBackedTableMetadataWriter.java:
##########
@@ -1360,7 +1361,10 @@ protected void compactIfNecessary(BaseHoodieWriteClient
writeClient, String late
// The compaction planner will manage to filter out the log files that
finished with greater completion time.
// see BaseHoodieCompactionPlanGenerator.generateCompactionPlan for more
details.
final String compactionInstantTime =
dataMetaClient.reloadActiveTimeline().filterInflightsAndRequested()
-
.findInstantsBeforeOrEquals(latestDeltacommitTime).firstInstant().map(HoodieInstant::getTimestamp)
+ .findInstantsBeforeOrEquals(latestDeltacommitTime).firstInstant()
+ // minus the pending instant time by 1 millisecond to avoid conflict
in case when this pending instant was finally been committed
+ // as a delta_commit in MDT.
+ .map(instant ->
HoodieInstantTimeGenerator.instantTimeMinusMillis(instant.getTimestamp(), 1L))
Review Comment:
Based on the logic, previously the compaction would never be scheduled,
given it picks the instant time that already exists (entering the if branch
logging `Compaction with same %s time is already present in the timeline.`).
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieInstantTimeGenerator.java:
##########
@@ -106,17 +106,18 @@ public static String instantTimePlusMillis(String
timestamp, long milliseconds)
}
public static String instantTimeMinusMillis(String timestamp, long
milliseconds) {
+ final String timestampInMillis = fixInstantTimeCompatibility(timestamp);
try {
- String timestampInMillis = fixInstantTimeCompatibility(timestamp);
- // To work with tests, that generate arbitrary timestamps, we need to
pad the timestamp with 0s.
- if (timestampInMillis.length() < MILLIS_INSTANT_TIMESTAMP_FORMAT_LENGTH)
{
- return String.format("%0" + timestampInMillis.length() + "d", 0);
- }
LocalDateTime dt = LocalDateTime.parse(timestampInMillis,
MILLIS_INSTANT_TIME_FORMATTER);
ZoneId zoneId = HoodieTimelineTimeZone.UTC.equals(commitTimeZone) ?
ZoneId.of("UTC") : ZoneId.systemDefault();
return
MILLIS_INSTANT_TIME_FORMATTER.format(dt.atZone(zoneId).toInstant().minusMillis(milliseconds).atZone(zoneId).toLocalDateTime());
} catch (DateTimeParseException e) {
- throw new HoodieException(e);
+ // To work with tests, that generate arbitrary timestamps, we need to
pad the timestamp with 0s.
+ if (isValidInstantTime(timestampInMillis)) {
+ return String.format("%0" + timestampInMillis.length() + "d",
Long.parseLong(timestampInMillis) - milliseconds);
+ } else {
+ throw new HoodieException(e);
+ }
Review Comment:
Given the new instant time meaning and completion time, do we allow
arbitrary timestamps any more (e.g., "00001", "00002", "000013)? Should we
create a follow-up ticket to fix all relevant tests?
--
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]
