[GitHub] [hudi] lokeshj1703 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
lokeshj1703 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1188380042
##
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:
Do you mean making this method synchronized or making
`HoodieInstantTimeGenerator` a singleton?
--
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] lokeshj1703 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
lokeshj1703 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1188380042
##
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:
Do you mean making this method synchronized?
--
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] lokeshj1703 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
lokeshj1703 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1188376891
##
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. We can handle it in a separate jira though.
--
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] lokeshj1703 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
lokeshj1703 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1188232721
##
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 have changed the PR to use metaClient's tableConfig. Please take a look.
I think `HoodieInstantTimeGenerator` usage needs some refactoring. The
static access doesn't seem right.
--
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] lokeshj1703 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
lokeshj1703 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1187365382
##
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 made it an option in the latest commit but I see some failures in Azure CI
where option value is not present for timeline timezone. Its in hudi-cli and
`HoodieJavaGenerateApp`, both of these use `HoodieInstantTimeGenerator`
directly without setting timezone.
--
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] lokeshj1703 commented on a diff in pull request #8631: [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain
lokeshj1703 commented on code in PR #8631:
URL: https://github.com/apache/hudi/pull/8631#discussion_r1185732010
##
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:
I can do a switch-case but I feel this is better. WDYT?
--
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
