-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
[Iceberg] cleanup FileIO resources #33509
Conversation
Assigning reviewers. If you would like to opt out of this review, comment R: @m-trieu for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
@@ -201,14 +200,15 @@ private void appendManifestFiles(Table table, Iterable<FileWriteResult> fileWrit | |||
} | |||
|
|||
private ManifestWriter<DataFile> createManifestWriter( | |||
String tableLocation, String uuid, PartitionSpec spec, FileIO io) { | |||
String tableLocation, String uuid, PartitionSpec spec, Table table) { |
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 we are passing Table
now we could drop tableLocation
and spec
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.
Thx for the catch. I refactored this a lil to make it more clean.
btw spec
here isn't a simple table.spec()
. This part of the code deals with the edge case where we have a batch of data files that were written with different specs (e.g. user updates spec during pipeline runtime). We group data files by spec and create/append one ManifestFile per spec.
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.
ah! nice makes sense
…nto cleanup_fileio
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!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #33509 +/- ##
============================================
+ Coverage 59.03% 60.40% +1.37%
- Complexity 3186 15154 +11968
============================================
Files 1146 2753 +1607
Lines 176129 267152 +91023
Branches 3366 12153 +8787
============================================
+ Hits 103975 161384 +57409
- Misses 68800 99316 +30516
- Partials 3354 6452 +3098
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This reverts commit 5a3ddc4.
Small PR to cleanup some resources that may be leaking and causing our Iceberg ITs to hang