-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-29268: Iceberg: Close catalog in Catalogs.loadTable() to fix resource leak from open ResolvingFileIO #6131
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
base: master
Are you sure you want to change the base?
Conversation
…ource leak from open ResolvingFileIO
} else { | ||
// fallback if not AutoCloseable | ||
return catalog.get().loadTable(TableIdentifier.parse(tableIdentifier)); | ||
} |
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 file is a copy of apache/iceberg/mr/.../Catalogs.java, and the upstream version does not have the equivalent of this change. Why does Apache Iceberg justify the current implementation? Or it is still a problem on upstream, but no MapReduce users are interested in the warning message.
The current implementation is likely to work. For future reference, I'm also curious about what the to-be solution is.
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 avoid logical fallback and close resource when required?
private static Table loadTable(Configuration conf, String tableIdentifier, String tableLocation,
String catalogName) {
Optional<Catalog> catalog = loadCatalog(conf, catalogName);
if (catalog.isPresent()) {
Preconditions.checkArgument(tableIdentifier != null, "Table identifier not set");
try {
return catalog.get().loadTable(TableIdentifier.parse(tableIdentifier));
} finally {
if (catalog.get() instanceof Closeable closeable) {
IOUtils.closeQuietly(closeable);
}
}
}
....
}
Preconditions.checkArgument(tableIdentifier != null, "Table identifier not set"); | ||
return catalog.get().loadTable(TableIdentifier.parse(tableIdentifier)); | ||
|
||
if (catalog.get() instanceof AutoCloseable) { |
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 guess Java 21 provides a smarter way to express this cast.
https://sonarcloud.io/project/issues?id=apache_hive&pullRequest=6131&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true
fceea4b
to
d6b7b35
Compare
I found that this proposed change resolves the warning for the unclosed
|
|
What changes were proposed in this pull request?
Added try-with-resources for a catalog that is loaded in Catalogs.loadTable().
Why are the changes needed?
Described in the description of HIVE-29268.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Manual check on the local Hive Docker instance.