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: commits-unsubscr...@hudi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org