Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danny0405 merged 3 commits into from
May 10, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
649fe6f
HUDI-6170. Use correct zone id while calculating earliestTimeToRetain
lokeshj1703 May 4, 2023
5e2f6a9
Address review comments
lokeshj1703 May 8, 2023
1652e6e
Use metaClient table config
lokeshj1703 May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions ...lient/hudi-client-common/src/main/java/org/apache/hudi/client/HoodieTimelineArchiver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@
import java.io.IOException;
import java.text.ParseException;
import java.time.Instant;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -491,7 +490,7 @@ private Stream<HoodieInstant> 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, metaClient.getTableConfig().getTimelineTimezone().getZoneId());
String earliestTimeToRetain = HoodieActiveTimeline.formatDate(Date.from(currentDateTime.minusHours(config.getCleanerHoursRetained()).toInstant()));

@danny0405 danny0405 May 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (HoodieTimeline.compareTimestamps(latestCommitToArchive, GREATER_THAN_OR_EQUALS, earliestTimeToRetain)) {
throw new HoodieIOException("Please align your archival configs based on cleaner configs. 'hoodie.keep.min.commits' : "
Expand Down
3 changes: 1 addition & 2 deletions ...ent/hudi-client-common/src/main/java/org/apache/hudi/table/action/clean/CleanPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@
import java.io.IOException;
import java.io.Serializable;
import java.time.Instant;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.util.ArrayList;
import java.util.Collections;
Expand Down Expand Up @@ -510,7 +509,7 @@ public Option<HoodieInstant> 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, hoodieTable.getMetaClient().getTableConfig().getTimelineTimezone().getZoneId());
String earliestTimeToRetain = HoodieActiveTimeline.formatDate(Date.from(currentDateTime.minusHours(hoursRetained).toInstant()));

@danny0405 danny0405 May 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nsivabalan nsivabalan May 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@danny0405 danny0405 May 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nsivabalan nsivabalan May 8, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lokeshj1703 lokeshj1703 May 8, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@danny0405 danny0405 May 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lokeshj1703 lokeshj1703 May 9, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@danny0405 danny0405 May 9, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lokeshj1703 lokeshj1703 May 9, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

earliestCommitToRetain = Option.fromJavaOptional(commitTimeline.getInstantsAsStream().filter(i -> HoodieTimeline.compareTimestamps(i.getTimestamp(),
HoodieTimeline.GREATER_THAN_OR_EQUALS, earliestTimeToRetain)).findFirst());
Expand Down
15 changes: 12 additions & 3 deletions hudi-common/src/main/java/org/apache/hudi/common/model/HoodieTimelineTimeZone.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

@danny0405 danny0405 May 5, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@lokeshj1703 lokeshj1703 May 5, 2023

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@danny0405 danny0405 May 10, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine, it's just a preference.


HoodieTimelineTimeZone(String timeZone) {
HoodieTimelineTimeZone(String timeZone, ZoneId zoneId) {
this.timeZone = timeZone;
this.zoneId = zoneId;
}

public String getTimeZone() {
return timeZone;
}

public ZoneId getZoneId() {
return zoneId;
}
}
4 changes: 4 additions & 0 deletions hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,10 @@ public String getKeyGeneratorClassName() {
return getString(KEY_GENERATOR_CLASS_NAME);
}

public HoodieTimelineTimeZone getTimelineTimezone() {
return HoodieTimelineTimeZone.valueOf(getStringOrDefault(TIMELINE_TIMEZONE));
}

public String getHiveStylePartitioningEnable() {
return getStringOrDefault(HIVE_STYLE_PARTITIONING_ENABLE);
}
Expand Down
9 changes: 9 additions & 0 deletions hudi-common/src/test/java/org/apache/hudi/common/table/timeline/TestHoodieInstant.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

package org.apache.hudi.common.table.timeline;

import org.apache.hudi.common.model.HoodieTimelineTimeZone;
import org.apache.hudi.common.testutils.HoodieCommonTestHarness;
import org.apache.hudi.common.util.Option;
import org.junit.jupiter.api.Test;
Expand All @@ -27,6 +28,7 @@

import static org.apache.hudi.common.testutils.Assertions.assertStreamEquals;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;

public class TestHoodieInstant extends HoodieCommonTestHarness {

Expand Down Expand Up @@ -76,4 +78,11 @@ public void testCreateHoodieInstantByFileStatus() throws IOException {
cleanMetaClient();
}
}

@Test
public void testHoodieTimelineTimeZone() {
for (HoodieTimelineTimeZone timeZone : HoodieTimelineTimeZone.values()) {
assertNotNull(timeZone.getZoneId());
}
}
}