-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
[HUDI-1381] Schedule compaction based on time elapsed #2260
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2260 +/- ##
=============================================
+ Coverage 50.27% 69.51% +19.24%
+ Complexity 2990 357 -2633
=============================================
Files 410 53 -357
Lines 18406 1929 -16477
Branches 1885 229 -1656
=============================================
- Hits 9253 1341 -7912
+ Misses 8395 456 -7939
+ Partials 758 132 -626
Flags with carried forward coverage won't be shown. Click here to find out more. |
4e81a88
to
679310a
Compare
@n3nash @vinothchandar hi ,can you pls take a look at this pr? |
@wangxianghu Can you review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a quick look, found two minor issues.
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/table/HoodieTable.java
Outdated
Show resolved
Hide resolved
...di-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestInlineCompaction.java
Outdated
Show resolved
Hide resolved
0750f24
to
6dac804
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Karl-WangSK Thanks for your contribution, I left some comments you can consider
...lient/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieWriteClient.java
Outdated
Show resolved
Hide resolved
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
5fcea7d
to
87a7489
Compare
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Karl-WangSK Thanks for addressing my concern, LGTM now!
cc @yanghua
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Karl-WangSK Left some comments.
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...di-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestInlineCompaction.java
Outdated
Show resolved
Hide resolved
...di-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestInlineCompaction.java
Outdated
Show resolved
Hide resolved
...di-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestInlineCompaction.java
Outdated
Show resolved
Hide resolved
...di-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestInlineCompaction.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
@yanghua ready to merge |
thanks for your reminder, will review it again~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Karl-WangSK Left some comments.
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
Outdated
Show resolved
Hide resolved
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
Outdated
Show resolved
Hide resolved
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Karl-WangSK Left some comments.
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
Outdated
Show resolved
Hide resolved
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieWriteConfig.java
Outdated
Show resolved
Hide resolved
...lient/hudi-client-common/src/main/java/org/apache/hudi/table/action/compact/CompactType.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Karl-WangSK Left some comments.
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java
Outdated
Show resolved
Hide resolved
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...di-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestInlineCompaction.java
Outdated
Show resolved
Hide resolved
...di-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestInlineCompaction.java
Outdated
Show resolved
Hide resolved
...di-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestInlineCompaction.java
Outdated
Show resolved
Hide resolved
...di-spark-client/src/test/java/org/apache/hudi/table/action/compact/TestInlineCompaction.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/exception/HoodieCompactException.java
Show resolved
Hide resolved
cc @yanghua |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall. Minor comments.
hudi-client/hudi-client-common/src/main/java/org/apache/hudi/config/HoodieCompactionConfig.java
Show resolved
Hide resolved
...ent-common/src/main/java/org/apache/hudi/table/action/compact/CompactionTriggerStrategy.java
Outdated
Show resolved
Hide resolved
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieActiveTimeline.java
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hudi/table/action/compact/SparkScheduleCompactionActionExecutor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Tips
What is the purpose of the pull request
Schedule compaction based on time elapsed
Brief change log
GH : #2229
It would be helpful to introduce configuration to schedule compaction based on time elapsed since last scheduled compaction.
Verify this pull request
This pull request is already covered by existing tests, such as (please describe tests).
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.