[HUDI-349]: Added new cleaning policy based on number of hours #3646

pratyakshsharma · 2021-09-12T13:38:18Z

Tips

Thank you very much for contributing to Apache Hudi.
Please review https://hudi.apache.org/contribute/how-to-contribute before opening a pull request.

What is the purpose of the pull request

Add a new cleaning policy based on the number of hours. This gives more flexibility for users compared to the one based on the number of commits

Brief change log

Added new cleaning policy
Added supporting methods

Verify this pull request

Added 2 test cases in TestCleaner class

(Please pick either of the following options)

This pull request is a trivial rework / code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end.
Added HoodieClientWriteTest to verify the change.
Manually verified the change by running a job locally.

Committer checklist

Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.

…or-hudi into hudi-349

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java

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

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

hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/TestCleaner.java

nsivabalan · 2021-09-22T00:52:57Z

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

+      earliestCommitToRetain = commitTimeline.nthInstant(commitTimeline.countInstants() - commitsRetained); //15 instants total, 10 commits to retain, this gives 6th instant in the list
+    } else if (config.getCleanerPolicy() == HoodieCleaningPolicy.KEEP_LAST_X_HOURS) {
+      Instant instant = Instant.now();
+      ZonedDateTime commitDateTime = ZonedDateTime.ofInstant(instant, ZoneId.systemDefault());


Do we have any precedence in hudi code base for doing time based calculations. Can you explore and let me know. Wanna maintain some uniformity if we have any.

Let me check.

We should sync this to how the instant times are generated.

Another way to think about this could be:
latestInstantTime = HoodieActiveTimeline.createNewInstantTime()
earliestCommitToRetain = HoodieActiveTimeline.findInstantInPast(latestInstantTime, N); // N = number of hours in the past.

Keeping the instant timestamp code together will prevent any surprises from a wrong time calculation cleaning data unintentionally.

@nsivabalan I checked. There is no uniformity as of now. We use joda dateTime library in TimestampBasedKeyGenerator, java.time.ZonedDateTime in SlashEncodedHourPartitionValueExtractor, and java.util.Date in HoodieActiveTimeline.

@prashantwason nthInstant is what we are using throughout the codebase. I guess there is no need to introduce a new method findInstantInPast here.

nsivabalan · 2021-10-06T01:05:24Z

@prashantwason : can you please address the feedback when you get a chance.

pratyakshsharma · 2021-10-11T08:24:15Z

@nsivabalan ack.

pratyakshsharma · 2021-11-10T20:05:32Z

@nsivabalan Please take a look.

veenaypatil · 2021-11-12T11:36:48Z

any update on this , I faced an issue today where the ETL failed in Prod because the file was deleted by Cleaner service

Caused by: java.io.FileNotFoundException: No such file or directory: s3a://bucket/cdcv2/data/a02b9653-d715-43a7-8faf-950cbdafebc4-0_123-95450-42475479_20211111205930.parquet
[2021-11-12 00:40:15,491] {logging_mixin.py:112} INFO - [2021-11-12 00:40:15,491] {batch.py:321} INFO - 	at org.apache.hadoop.fs.s3a.S3AFileSystem.s3GetFileStatus(S3AFileSystem.java:2269)

Having config based on time will help instead of relying on Number of commits
cc @vinothchandar

nsivabalan · 2021-12-16T20:43:11Z

@pratyakshsharma : Can you update the patch when you get a chance.

pratyakshsharma · 2021-12-21T14:56:31Z

@nsivabalan Please take a pass.

vinothchandar · 2021-12-25T19:32:25Z

Let me review this as well

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java

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

nsivabalan · 2022-01-24T03:33:17Z

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

+          } else if (policy == HoodieCleaningPolicy.KEEP_LATEST_BY_HOURS) {
+            // This block corresponds to KEEP_LATEST_BY_HOURS policy
+            // Do not delete the latest commit.
+            if (fileCommitTime.equals(lastVersion)) {


Do we know why do have (fileCommitTime.equals(lastVersionBeforeEarliestCommitToRetain) in L318. I would prefer to keep the same logic unless we have strong reasons to remove. As I mentioned, cleaning based on num hours is just a diff way to conveying retain N commits. So, lets try to keep the logic similar across both. Easier to maintain.

The cleaner policies are designed to enable the longest running query to succeed. Let us take a small example where ingestion is happening every 30 minutes and hoodie.cleaner.commits.retained = 2. This implies the longest running query is going to take 1 hour to complete. Now if I maintain a list of commits, when commit at index=2 in this list (i.e 3rd commit from the beginning) is completed, the query which started when commit at index=0 completed might still be running.

list of commits -> 0. 1. 2
time (in mins). -> 0. 30. 60

Hence to allow this query to finish, we take into account lastVersionBeforeEarliestCommitToRetain in L318. lastVersion in this case is going to be commit at index=1.

With the new policy KEEP_LATEST_BY_HOURS, when 3rd commit is completed, 1st commit will be retained automatically by the logic. Hence the code in its current form works fine.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java

hudi-client/hudi-spark-client/src/test/java/org/apache/hudi/table/TestCleaner.java

nsivabalan · 2022-02-04T05:24:29Z

@prashantwason : ping

prashantwason · 2022-02-04T07:59:03Z

@prashantwason : ping

@nsivabalan Did you intend to ping @pratyakshsharma so he can respond to the comments? For me, this is on my review list but am a bit behind due to metadata index related PRs.

pratyakshsharma · 2022-02-08T08:02:11Z

@nsivabalan @prashantwason

I had been waiting for @vinothchandar 's review on this. Let me address the existing comments so we can land this.

pratyakshsharma · 2022-02-08T10:23:14Z

@hudi-bot run azure

pratyakshsharma · 2022-02-08T10:23:40Z

@nsivabalan Please take a pass.

nsivabalan · 2022-02-10T05:47:38Z

@pratyakshsharma : Can you check for CI failures. also, is it already rebased w/ latest master.

nsivabalan

As suggested in other patch, can we try to keep the tests to TestCleanActionExecutor if possible.

hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java

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

nsivabalan · 2022-02-10T05:59:47Z

thanks for fixing all refactoring feedback. now the source code changes looks a lot less.

nsivabalan · 2022-02-15T04:43:10Z

@pratyakshsharma : once you address the feedback, let me know.

nsivabalan

LGTM. wrt tests as suggested, can we move the tests to CleanActionExecutor tests rather than at write client layer. Is there any challenges with that?

pratyakshsharma · 2022-02-20T11:11:27Z

.../hudi-spark-client/src/test/java/org/apache/hudi/table/functional/TestCleanPlanExecutor.java

+   */
+  @ParameterizedTest
+  @MethodSource("argumentsForTestKeepLatestCommits")
+  public void testKeepXHoursWithCleaning(boolean simulateFailureRetry, boolean enableIncrementalClean, boolean enableBootstrapSourceClean) throws Exception {


I have moved only one test in this PR since the rest are already moved in #4385. Once that is merged, changes will automatically reflect here as well. @nsivabalan

hudi-bot · 2022-02-20T12:12:19Z

CI report:

cc0a44a Azure: SUCCESS

Bot commands

@hudi-bot supports the following commands:

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

…e#3646)

pratyakshsharma added 7 commits May 27, 2021 23:00

[HUDI-349]: dev in progress

97113ff

added cleaner hours retained function

92d085d

checkstyle fixes

9c1364c

[HUDI-349]: added new cleaning policy

f93ee61

Merge branch 'hudi-349' of https://github.com/pratyakshsharma/incubat…

0cd1fe6

…or-hudi into hudi-349

[HUDI-349]: rebased with master

68b0a68

[HUDI-349]: small changes

f58beda

pratyakshsharma requested review from vinothchandar and nsivabalan September 12, 2021 14:27

nsivabalan requested changes Sep 22, 2021

View reviewed changes

vinothchandar added this to Ready for Review in PR Tracker Board Sep 22, 2021

vinothchandar assigned vinothchandar and nsivabalan Sep 23, 2021

code review comments addressed

3c79136

[HUDI-349]: in sync with master

676d999

nsivabalan requested changes Jan 24, 2022

View reviewed changes

PR Tracker Board automation moved this from Ready for Review to Nearing Landing Jan 24, 2022

nsivabalan added the priority:critical production down; pipelines stalled; Need help asap. label Feb 8, 2022

[HUDI-349]: addressed code review comments

29e8200

nsivabalan requested changes Feb 10, 2022

View reviewed changes

pratyakshsharma added 3 commits February 17, 2022 01:19

Merge branch 'master' of https://github.com/apache/hudi into hudi-349

9527702

updated documentation for CLEANER_HOURS_RETAINED prop

efa6056

build fix

7eea9ec

nsivabalan reviewed Feb 18, 2022

View reviewed changes

pratyakshsharma added 2 commits February 20, 2022 16:19

Merge branch 'master' of https://github.com/apache/hudi into hudi-349

8e7e497

[HUDI-349]: moved tests to new test class

cc0a44a

pratyakshsharma commented Feb 20, 2022

View reviewed changes

nsivabalan approved these changes Feb 21, 2022

View reviewed changes

nsivabalan merged commit bf16bc1 into apache:master Feb 21, 2022

PR Tracker Board automation moved this from Nearing Landing to Done Feb 21, 2022

nsivabalan added the on-call-triaged label Feb 27, 2022

vingov pushed a commit to vingov/hudi that referenced this pull request Apr 3, 2022

[HUDI-349]: Added new cleaning policy based on number of hours (apach…

b5ee38a

…e#3646)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HUDI-349]: Added new cleaning policy based on number of hours #3646

[HUDI-349]: Added new cleaning policy based on number of hours #3646

pratyakshsharma commented Sep 12, 2021 •

edited

Loading

nsivabalan Sep 22, 2021

pratyakshsharma Oct 11, 2021

prashantwason Oct 21, 2021

pratyakshsharma Oct 26, 2021

nsivabalan commented Oct 6, 2021

pratyakshsharma commented Oct 11, 2021

pratyakshsharma commented Nov 10, 2021

veenaypatil commented Nov 12, 2021

nsivabalan commented Dec 16, 2021

pratyakshsharma commented Dec 21, 2021

vinothchandar commented Dec 25, 2021

nsivabalan Jan 24, 2022

pratyakshsharma Feb 8, 2022

nsivabalan commented Feb 4, 2022

prashantwason commented Feb 4, 2022

pratyakshsharma commented Feb 8, 2022 •

edited

Loading

pratyakshsharma commented Feb 8, 2022

pratyakshsharma commented Feb 8, 2022

nsivabalan commented Feb 10, 2022

nsivabalan left a comment

nsivabalan commented Feb 10, 2022

nsivabalan commented Feb 15, 2022

nsivabalan left a comment

pratyakshsharma Feb 20, 2022

hudi-bot commented Feb 20, 2022

[HUDI-349]: Added new cleaning policy based on number of hours #3646

[HUDI-349]: Added new cleaning policy based on number of hours #3646

Conversation

pratyakshsharma commented Sep 12, 2021 • edited Loading

Tips

What is the purpose of the pull request

Brief change log

Verify this pull request

Committer checklist

nsivabalan Sep 22, 2021

Choose a reason for hiding this comment

pratyakshsharma Oct 11, 2021

Choose a reason for hiding this comment

prashantwason Oct 21, 2021

Choose a reason for hiding this comment

pratyakshsharma Oct 26, 2021

Choose a reason for hiding this comment

nsivabalan commented Oct 6, 2021

pratyakshsharma commented Oct 11, 2021

pratyakshsharma commented Nov 10, 2021

veenaypatil commented Nov 12, 2021

nsivabalan commented Dec 16, 2021

pratyakshsharma commented Dec 21, 2021

vinothchandar commented Dec 25, 2021

nsivabalan Jan 24, 2022

Choose a reason for hiding this comment

pratyakshsharma Feb 8, 2022

Choose a reason for hiding this comment

nsivabalan commented Feb 4, 2022

prashantwason commented Feb 4, 2022

pratyakshsharma commented Feb 8, 2022 • edited Loading

pratyakshsharma commented Feb 8, 2022

pratyakshsharma commented Feb 8, 2022

nsivabalan commented Feb 10, 2022

nsivabalan left a comment

Choose a reason for hiding this comment

nsivabalan commented Feb 10, 2022

nsivabalan commented Feb 15, 2022

nsivabalan left a comment

Choose a reason for hiding this comment

pratyakshsharma Feb 20, 2022

Choose a reason for hiding this comment

hudi-bot commented Feb 20, 2022

CI report:

pratyakshsharma commented Sep 12, 2021 •

edited

Loading

pratyakshsharma commented Feb 8, 2022 •

edited

Loading