-
Notifications
You must be signed in to change notification settings - Fork 2.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
Hive: close the fileIO client when closing the hive catalog #10771
Hive: close the fileIO client when closing the hive catalog #10771
Conversation
@Override | ||
public void close() throws IOException { | ||
super.close(); | ||
fileIO.close(); |
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.
fileIO
could be null at this point if initialization failed and close is called immediately after it, so it would be good to add a null check.
Generally speaking, there's no guarantee that adding close()
here actually fixes the exception you mentioned in the PR. You would most likely need a similar concept to what was introduced by #7487 / #8315
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 just created a fileIOCloser
to track the IO files and close them when the catalog is closed, could your recheck please?
7bc0d3d
to
9917212
Compare
Looks like the flink test failure is unrelated, I reopened #10356 since it looks like that test is still flaky. I'm retriggering CI. |
9917212
to
f896ed6
Compare
HiveTableOperations hiveTableOperations = | ||
new HiveTableOperations(conf, clients, fileIO, name, dbName, tableName); | ||
fileIOCloser.put(hiveTableOperations, hiveTableOperations.io()); |
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.
Something I've wondered for a bit, is it possible to uplevel any of this logic to BaseMetastoreCatalog
so that we avoid having every catalog which extends from that to have it's own Cache<TableOperations, FileIO>?
Or does that not generalize? Not a blocker, but just wanted to make sure we at least thought through it to avoid duplication.
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 tried to move it to uplevel but I found some subclasses without a FileIO instance (ViewAwareTableBuilder
and HadoopCatalogTableBuilder
), if you have any suggestions to generalize it, I'm open to 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 have a TODO on my list to generalize this, so I'll take a look at this (hopefully this week)
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.
@amogh-jahagirdar I've opened #10893 to addresse this
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java
Outdated
Show resolved
Hide resolved
…og.java Co-authored-by: Eduard Tudenhoefner <[email protected]>
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java
Outdated
Show resolved
Hide resolved
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java
Outdated
Show resolved
Hide resolved
Thanks @hussein-awala , and thank you @nastra for reviewing! |
This PR fixes the following warning: