-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Core: Create commit groups in commit service offer in RewriteDataFilesCommitManager #6539
Core: Create commit groups in commit service offer in RewriteDataFilesCommitManager #6539
Conversation
4c373c1
to
8ea8bb8
Compare
8ea8bb8
to
5476e70
Compare
Is there any new test case we can add to ensure correctness of this? |
Sure @jackye1995, presently I didn't add new ut's to this pr as the ut i was thinking of adding :
|
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.
looks good to me
@@ -264,5 +253,35 @@ public void close() { | |||
"File groups offered after service was closed, " | |||
+ "they were not successfully committed."); | |||
} | |||
|
|||
private void commitReadyCommitGroups() { | |||
if (canCreateCommitGroup()) { |
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 this is a good way to do things, I was considering having some kind of atomic countdown but I like this as well
core/src/main/java/org/apache/iceberg/actions/RewriteDataFilesCommitManager.java
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.
Question on the size of the Synchronization block
8476fa7
to
30d48d5
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.
Looks good to me now!
Thanks @singhpk234 for the code, and @jackye1995 for the review! |
About the change
Implements the idea proposed in #6367
Testing done
Existing UT's
cc @RussellSpitzer @flyrain