[GitHub] [hudi] danny0405 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
danny0405 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1189298111
##
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieTimelineTimeZone.java:
##
@@ -18,20 +18,29 @@
package org.apache.hudi.common.model;
+import java.time.ZoneId;
+import java.util.TimeZone;
+
/**
* Hoodie TimelineZone.
*/
public enum HoodieTimelineTimeZone {
- LOCAL("local"),
- UTC("utc");
+ LOCAL("local", ZoneId.systemDefault()),
+ UTC("utc", TimeZone.getTimeZone("UTC").toZoneId());
private final String timeZone;
+ private final ZoneId zoneId;
Review Comment:
Fine, it's just a preference.
--
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
[GitHub] [hudi] danny0405 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
danny0405 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1188264786
##
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieInstantTimeGenerator.java:
##
@@ -135,7 +133,7 @@ private static TemporalAccessor
convertDateToTemporalAccessor(Date d) {
}
public static void setCommitTimeZone(HoodieTimelineTimeZone commitTimeZone) {
-commitTimeZoneOpt = Option.of(commitTimeZone);
+HoodieInstantTimeGenerator.commitTimeZone = commitTimeZone;
Review Comment:
Should make it singleton by using a `synchromized` lock
--
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
[GitHub] [hudi] danny0405 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
danny0405 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1188261901
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##
@@ -510,7 +510,7 @@ public Option getEarliestCommitToRetain() {
}
} else if (config.getCleanerPolicy() ==
HoodieCleaningPolicy.KEEP_LATEST_BY_HOURS) {
Instant instant = Instant.now();
- ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
ZoneId.systemDefault());
+ ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
HoodieInstantTimeGenerator.getTimelineTimeZone().getZoneId());
String earliestTimeToRetain =
HoodieActiveTimeline.formatDate(Date.from(currentDateTime.minusHours(hoursRetained).toInstant()));
Review Comment:
> I think HoodieInstantTimeGenerator usage needs some refactoring
Yes, maybe we should pass in the table path as param and triggers a lazy
initialization for the zoneId if necessary.
--
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
[GitHub] [hudi] danny0405 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
danny0405 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1188063597
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##
@@ -510,7 +510,7 @@ public Option getEarliestCommitToRetain() {
}
} else if (config.getCleanerPolicy() ==
HoodieCleaningPolicy.KEEP_LATEST_BY_HOURS) {
Instant instant = Instant.now();
- ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
ZoneId.systemDefault());
+ ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
HoodieInstantTimeGenerator.getTimelineTimeZone().getZoneId());
String earliestTimeToRetain =
HoodieActiveTimeline.formatDate(Date.from(currentDateTime.minusHours(hoursRetained).toInstant()));
Review Comment:
Yeah, that means the code is prone to making misusages, let's fix all those
test falures by initialzing the zoneId manually.
--
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
[GitHub] [hudi] danny0405 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
danny0405 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1188063597
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##
@@ -510,7 +510,7 @@ public Option getEarliestCommitToRetain() {
}
} else if (config.getCleanerPolicy() ==
HoodieCleaningPolicy.KEEP_LATEST_BY_HOURS) {
Instant instant = Instant.now();
- ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
ZoneId.systemDefault());
+ ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
HoodieInstantTimeGenerator.getTimelineTimeZone().getZoneId());
String earliestTimeToRetain =
HoodieActiveTimeline.formatDate(Date.from(currentDateTime.minusHours(hoursRetained).toInstant()));
Review Comment:
Yeah, that means the code is prune to making misusages, let's fix all those
test falures by initialzing the zoneId manually.
--
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
[GitHub] [hudi] danny0405 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
danny0405 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1185813955
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##
@@ -510,7 +510,7 @@ public Option getEarliestCommitToRetain() {
}
} else if (config.getCleanerPolicy() ==
HoodieCleaningPolicy.KEEP_LATEST_BY_HOURS) {
Instant instant = Instant.now();
- ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
ZoneId.systemDefault());
+ ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
HoodieInstantTimeGenerator.getTimelineTimeZone().getZoneId());
String earliestTimeToRetain =
HoodieActiveTimeline.formatDate(Date.from(currentDateTime.minusHours(hoursRetained).toInstant()));
Review Comment:
> tableConfig is generated for the first time when table is created
Can we always ensure table config is initialized first? How to guard this
sequence dependency.
--
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
[GitHub] [hudi] danny0405 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
danny0405 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1185653080
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java:
##
@@ -491,7 +491,7 @@ private Stream getCommitInstantsToArchive()
throws IOException {
String latestCommitToArchive =
instantsToArchive.get(instantsToArchive.size() - 1).getTimestamp();
try {
Instant latestCommitInstant =
HoodieActiveTimeline.parseDateFromInstantTime(commitTimeline.lastInstant().get().getTimestamp()).toInstant();
- ZonedDateTime currentDateTime =
ZonedDateTime.ofInstant(latestCommitInstant, ZoneId.systemDefault());
+ ZonedDateTime currentDateTime =
ZonedDateTime.ofInstant(latestCommitInstant,
HoodieInstantTimeGenerator.getTimelineTimeZone().getZoneId());
String earliestTimeToRetain =
HoodieActiveTimeline.formatDate(Date.from(currentDateTime.minusHours(config.getCleanerHoursRetained()).toInstant()));
Review Comment:
The code only works when `HoodieInstantTimeGenerator.setCommitTimeZone` is
executed in the same JVM process, we must asure that. Either set it up
explicitly or instantiate a `HoodieTableConfig` within which the timezone is
set up.
##
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java:
##
@@ -510,7 +510,7 @@ public Option getEarliestCommitToRetain() {
}
} else if (config.getCleanerPolicy() ==
HoodieCleaningPolicy.KEEP_LATEST_BY_HOURS) {
Instant instant = Instant.now();
- ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
ZoneId.systemDefault());
+ ZonedDateTime currentDateTime = ZonedDateTime.ofInstant(instant,
HoodieInstantTimeGenerator.getTimelineTimeZone().getZoneId());
String earliestTimeToRetain =
HoodieActiveTimeline.formatDate(Date.from(currentDateTime.minusHours(hoursRetained).toInstant()));
Review Comment:
The code only works when `HoodieInstantTimeGenerator.setCommitTimeZone` is
executed in the same JVM process, we must asure that. Either set it up
explicitly or instantiate a `HoodieTableConfig` within which the timezone is
set up.
--
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
[GitHub] [hudi] danny0405 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
danny0405 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1185651903
##
hudi-common/src/main/java/org/apache/hudi/common/model/HoodieTimelineTimeZone.java:
##
@@ -18,20 +18,29 @@
package org.apache.hudi.common.model;
+import java.time.ZoneId;
+import java.util.TimeZone;
+
/**
* Hoodie TimelineZone.
*/
public enum HoodieTimelineTimeZone {
- LOCAL("local"),
- UTC("utc");
+ LOCAL("local", ZoneId.systemDefault()),
+ UTC("utc", TimeZone.getTimeZone("UTC").toZoneId());
private final String timeZone;
+ private final ZoneId zoneId;
Review Comment:
Don't think we need a zoneId member, can we calculate it on the fly in
method `getZoneId` ?
--
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
