-
Notifications
You must be signed in to change notification settings - Fork 298
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
[AMORO-2257] Automatically create tags on snapshots hour-level for iceberg Table Formats #2411
Conversation
huyuanfeng seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2411 +/- ##
============================================
- Coverage 32.26% 32.24% -0.02%
+ Complexity 4384 4379 -5
============================================
Files 589 589
Lines 49867 49874 +7
Branches 6615 6616 +1
============================================
- Hits 16088 16081 -7
- Misses 32510 32522 +12
- Partials 1269 1271 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
f97237c
to
abf96b0
Compare
It seems that we forget to implement this method in @Override
public void handleConfigChanged(TableRuntime tableRuntime, TableConfiguration originalConfig) {
scheduleIfNecessary(tableRuntime, getStartDelay());
} Can you add this method to this PR? @huyuanfeng2018 |
ok |
ams/server/src/main/java/com/netease/arctic/server/table/TagConfiguration.java
Outdated
Show resolved
Hide resolved
9e3a862
to
10690c8
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.
@huyuanfeng2018 I left some comments, please take a look.
ams/server/src/main/java/com/netease/arctic/server/table/TagConfiguration.java
Outdated
Show resolved
Hide resolved
ams/server/src/main/java/com/netease/arctic/server/table/TagConfiguration.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/netease/arctic/table/TableProperties.java
Outdated
Show resolved
Hide resolved
done. |
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.
Thanks for you contribution! @huyuanfeng2018
The code looks fine to me, only with some details needs to be modified.
core/src/main/java/com/netease/arctic/table/TableProperties.java
Outdated
Show resolved
Hide resolved
ams/server/src/main/java/com/netease/arctic/server/table/TagConfiguration.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!
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.
@huyuanfeng2018 @wangtaohz
I left some comments, please take another look.
docs/user-guides/configurations.md
Outdated
| tag.auto-create.trigger.period | daily | Period of creating tags, support `daily`,`hourly` now | | ||
| tag.auto-create.trigger.offset.minutes | 0 | The minutes by which the tag is created after midnight (00:00) | | ||
| tag.auto-create.trigger.max-delay.minutes | 60 | The maximum delay time for creating a tag | | ||
| tag.auto-create.tag-format | 'tag-'yyyyMMdd for daily and 'tag-'yyyyMMddHH for hourly periods | The format of the <br/>name for daily tag <br> | |
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.
The description here seems to be misleading.
IMO, we can set the format for both daily and hourly auto-tag functions with this configuration.
However, I'm curious as to whether I need to carefully set different values when I configure this configuration based on different trigger periods. For example, in the daily mode, if it's set as 'tag-'yyyyMMddHH
, will auto-tag still function properly, or in the hourly mode, if it's set as 'tag-'yyyyMMdd
.
throw new IllegalArgumentException( | ||
"unsupported trigger period " + tagConfig.getTriggerPeriod()); | ||
} | ||
LocalDateTime tagTime = |
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, when I reviewed the code here, these three time-related variables made it difficult for me to understand. After carefully reviewing the code again, I have summarized the names and meanings of the three parameters:
- Check time: the timestamp when the action is started.
- Tag time: the timestamp of the last tag represents.
- Trigger time: the timestamp of the last tag should be triggered.
And we calculated the 3 variables with the methods:
- checkTime = now()
- triggerTime = (checkTime - triggerOffset) % period + triggerOffest
- TagTime = triggerTime - triggerOffest - period
then,
- TagTime = (checkTime - triggerOffset) % period - period
- TriggerTime = TagTime + period + triggerOffset
However, the tag time here does not correspond to the time of the tag. It is easier for us to generate the corresponding tag name based on this tag time, but it does not accurately represent the data within the tag.
We should calculate the 3 variables like :
- checkTime = now()
- TagTime = (checkTime - triggerOffset) % period
- triggerTime = TagTime + triggerOffest
Then the value of the tag time is more accurate and the calculation methods are much simpler.
So I think we can make the code much easier to understand by:
- Put the 3 variables together and try to calculate them at the beginning, put some comments to tell the meaning and the calculating rules.
- Calculate the tag time first and then calculate the trigger time based on tag time.
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.
Thanks a lot for your great work!
Automatically create tags on snapshots hour-level for iceberg Table Formats
Why are the changes needed?
Close #2257.
Brief change log
TagConfiguration.Period
How was this patch tested?
Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request
Documentation