-
Notifications
You must be signed in to change notification settings - Fork 5.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
Support procedure expire_snapshots for iceberg #22609
Support procedure expire_snapshots for iceberg #22609
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff 152f962...5fbfac8. No notifications. |
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 the doc! Looks good, I made a few suggestions. Let me know what you think.
``retain_last`` int Number of ancestor snapshots to preserve regardless of older_than | ||
(defaults to 1) | ||
|
||
``snapshot_ids`` array of long Array of snapshot IDs to expire |
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.
Is a word missing after "array of long"?
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.
No, what I want to express is that the type is array of long
. Is it OK with this expression in your opinion?
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.
That's fine! I was unfamiliar with the phrase and I should have looked it up first. This is good as is. Thanks!
018a29c
to
efb9853
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.
LGTM! (docs)
Pull updated branch, new local docs build, everything looks good. Thanks!
Suggest minor changes to the release note entry, following the Order of Changes and Section naming in Release Notes Guidelines.
|
@steveburnett Thanks for the standardization of the release note, fixed! |
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! All the expiry options seem at first like they're going to require a lot of explanation, but it's very clear and concise 😄
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.
Nice work! Just a couple of questions.
...iceberg/src/test/java/com/facebook/presto/iceberg/procedure/TestExpireSnapshotProcedure.java
Outdated
Show resolved
Hide resolved
|
||
protected long waitUntilAfter(long timestampMillis) | ||
{ | ||
long current = System.currentTimeMillis(); |
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.
This is run in a hot loop. I don't see where we actually wait though--this seems to always wait until after the timestamp of the snapshot. Do we need it?
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.
I think we need this. Iceberg's ExpireSnapshots.expireOlderThan(timestamp)
will expires all snapshots older than the given milliseconds timestamp, that means the snapshot with the same timestamp would not be expired. So for the sake of stability in testing, we'd better wait for this millisecond to pass, in case that all things happen in the same millisecond.
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.
Can we not just use Thread.sleep(1)
in place of this?
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.
Oh, I referred to Iceberg's test cases which use this spinning wait. In my opinion, when the wait time is very short, for example shorter than 1ms in this case, a spinning wait may be better than a block wait. It could avoid thread scheduling and context switching which may be triggered by Thread.sleep(ts)
, that might cause the waiting time more longer than expected.
Besides, should we obey the test standards to avoid using Thread.sleep
? Referring to https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md?plain=1#L374
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.
If it's shorter than 1ms, let's add a precondition which asserts that, so that the test fails rather than busy waits? Perhaps a boundary of ~10ms is reasonable?
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.
Sure, that makes sense! Fixed.
7df0a18
efb9853
to
7df0a18
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.
a few minor things, otherwise LGTM
...iceberg/src/test/java/com/facebook/presto/iceberg/procedure/TestExpireSnapshotProcedure.java
Outdated
Show resolved
Hide resolved
|
||
protected long waitUntilAfter(long timestampMillis) | ||
{ | ||
long current = System.currentTimeMillis(); |
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.
Can we not just use Thread.sleep(1)
in place of this?
938433a
to
c55b898
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.
Just one nit, otherwise LGTM
...iceberg/src/test/java/com/facebook/presto/iceberg/procedure/TestExpireSnapshotProcedure.java
Outdated
Show resolved
Hide resolved
c55b898
to
5fbfac8
Compare
Description
This PR support the procedure
expire_snapshots
for iceberg. It can be used to remove older snapshots or specified snapshots and their files which are no longer needed.See examples as follow:
Motivation and Context
Support expiring snapshots for iceberg
Test Plan
TestExpireSnapshotProcedure
Contributor checklist
Release Notes