[HUDI-6170] Use correct zone id while calculating earliestTimeToRetain #8631

lokeshj1703 · 2023-05-04T10:03:44Z

Change Logs

Currently we use default zoneId while calculating earliestTimeToRetain. Jira aims to use the configured timezone.

Impact

NA

Risk level (write none, low medium or high below)

low

Documentation Update

NA

Contributor's checklist

Read through contributor's guide
Change Logs and Impact were stated clearly
Adequate tests were added if applicable
CI passed

danny0405 · 2023-05-05T02:19:53Z

hudi-common/src/main/java/org/apache/hudi/common/model/HoodieTimelineTimeZone.java

+  UTC("utc", TimeZone.getTimeZone("UTC").toZoneId());

  private final String timeZone;
+  private final ZoneId zoneId;


Don't think we need a zoneId member, can we calculate it on the fly in method getZoneId ?

I can do a switch-case but I feel this is better. WDYT?

Fine, it's just a preference.

danny0405 · 2023-05-05T02:24:21Z

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java

          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()));


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.

danny0405 · 2023-05-05T02:24:28Z

...client/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java

      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()));


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.

tableConfig is generated for the first time when table is created and the time zone is set right then. after that, we can't alter the time zone.
This is in line w/ how we generate our commit times as well.

not sure if we need any fixes here. can you help clarify

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.

I see the issue. may be we should not set any default value for commitTimeZone in HoodieInstantTimeGenerator. we can set it to null infact. And so unless the table properties are instantiation whcih in turn will call into setCommitTimeZone, if any other callers tries to access the commit time zone, will fail fast.

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.

Yeah, that means the code is prone to making misusages, let's fix all those test falures by initialzing the zoneId manually.

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.

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.

Yeah. We can handle it in a separate jira though.

nsivabalan

left a suggestion

danny0405 · 2023-05-09T07:50:01Z

hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieInstantTimeGenerator.java


  public static void setCommitTimeZone(HoodieTimelineTimeZone commitTimeZone) {
-    commitTimeZoneOpt = Option.of(commitTimeZone);
+    HoodieInstantTimeGenerator.commitTimeZone = commitTimeZone;


Should make it singleton by using a synchromized lock

Do you mean making this method synchronized or making HoodieInstantTimeGenerator a singleton?

hudi-bot · 2023-05-09T16:15:11Z

CI report:

1652e6e UNKNOWN
b6d4996 Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

@hudi-bot run azure re-run the last Azure build

danny0405

+1, thanks for the contribution @lokeshj1703 ~

apache#8631) * Use correct zone id while calculating earliestTimeToRetain * Use metaClient table config

lokeshj1703 marked this pull request as ready for review May 4, 2023 10:04

lokeshj1703 changed the title ~~HUDI-6170. Use correct zone id while calculating earliestTimeToRetain~~ [HUDI-6170] Use correct zone id while calculating earliestTimeToRetain May 4, 2023

danny0405 reviewed May 5, 2023

View reviewed changes

danny0405 self-assigned this May 5, 2023

danny0405 added writer-core area:table-service Table services labels May 5, 2023

nsivabalan reviewed May 8, 2023

View reviewed changes

lokeshj1703 added 2 commits May 8, 2023 17:17

HUDI-6170. Use correct zone id while calculating earliestTimeToRetain

649fe6f

Address review comments

5e2f6a9

lokeshj1703 force-pushed the HUDI-6170 branch from 91d8e92 to 5e2f6a9 Compare May 8, 2023 11:47

Use metaClient table config

1652e6e

danny0405 reviewed May 9, 2023

View reviewed changes

lokeshj1703 force-pushed the HUDI-6170 branch from b6d4996 to 1652e6e Compare May 9, 2023 09:29

danny0405 approved these changes May 10, 2023

View reviewed changes

danny0405 merged commit 92e52dc into apache:master May 10, 2023

danny0405 mentioned this pull request May 11, 2023

[HUDI-6155] Fix cleaner based on hours for earliest commit to retain #8659

Closed

4 tasks

lokeshj1703 mentioned this pull request May 11, 2023

[SUPPORT] Spark SQL query FileNotFoundException using cleaner policy KEEP_LATEST_BY_HOURS #8584

Closed

yihua pushed a commit to yihua/hudi that referenced this pull request May 15, 2023

[HUDI-6170] Use correct zone id while calculating earliestTimeToRetain (

3635950

apache#8631) * Use correct zone id while calculating earliestTimeToRetain * Use metaClient table config

yihua pushed a commit to yihua/hudi that referenced this pull request May 15, 2023

[HUDI-6170] Use correct zone id while calculating earliestTimeToRetain (

a8ed3d1

apache#8631) * Use correct zone id while calculating earliestTimeToRetain * Use metaClient table config

yihua pushed a commit to yihua/hudi that referenced this pull request May 17, 2023

[HUDI-6170] Use correct zone id while calculating earliestTimeToRetain (

b169f78

apache#8631) * Use correct zone id while calculating earliestTimeToRetain * Use metaClient table config

[HUDI-6170] Use correct zone id while calculating earliestTimeToRetain #8631

[HUDI-6170] Use correct zone id while calculating earliestTimeToRetain #8631

Uh oh!

Conversation

lokeshj1703 commented May 4, 2023

Change Logs

Impact

Risk level (write none, low medium or high below)

Documentation Update

Contributor's checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danny0405 May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsivabalan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lokeshj1703 May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hudi-bot commented May 9, 2023

CI report:

Uh oh!

danny0405 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

danny0405 May 9, 2023 •

edited

Loading

lokeshj1703 May 9, 2023 •

edited

Loading