You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/06/30 13:32:43 UTC
[GitHub] [iceberg] NeQuissimus opened a new pull request, #5166: Close resources underlying various classes
NeQuissimus opened a new pull request, #5166:
URL: https://github.com/apache/iceberg/pull/5166
The underlying objects implement `close()`, which should be called
when HiveCatalog is discarded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5166: HiveCatalog: Close resources underlying various classes
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5166:
URL: https://github.com/apache/iceberg/pull/5166#discussion_r911088095
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -547,6 +547,11 @@ protected Map<String, String> properties() {
return catalogProperties == null ? ImmutableMap.of() : catalogProperties;
}
+ public void close() {
+ fileIO.close();
+ clients.close();
Review Comment:
The current practice is that we use `CatalogUtil.loadCatalog` to create the Catalog every time when we load a table. Opening and closing the clientPool (HMSClients) every time would be a costly operation, and we would likely lose everything that we win by using a pool.
Basically the expectation is that we have a global catalog cache and a thin Catalog wrapper using it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] NeQuissimus closed pull request #5166: HiveCatalog: Close resources underlying various classes
Posted by GitBox <gi...@apache.org>.
NeQuissimus closed pull request #5166: HiveCatalog: Close resources underlying various classes
URL: https://github.com/apache/iceberg/pull/5166
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5166: HiveCatalog: Close resources underlying various classes
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5166:
URL: https://github.com/apache/iceberg/pull/5166#discussion_r911078152
##########
core/src/main/java/org/apache/iceberg/hadoop/HadoopFileIO.java:
##########
@@ -73,8 +73,7 @@ public OutputFile newOutputFile(String path) {
@Override
public void deleteFile(String path) {
Path toDelete = new Path(path);
- FileSystem fs = Util.getFs(toDelete, hadoopConf.get());
- try {
+ try (FileSystem fs = Util.getFs(toDelete, hadoopConf.get())) {
Review Comment:
This will remove the FileSystem from the cache.
If we delete multiple files on S3, closing the FS every time will become costly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] NeQuissimus commented on a diff in pull request #5166: HiveCatalog: Close resources underlying various classes
Posted by GitBox <gi...@apache.org>.
NeQuissimus commented on code in PR #5166:
URL: https://github.com/apache/iceberg/pull/5166#discussion_r911112520
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -547,6 +547,11 @@ protected Map<String, String> properties() {
return catalogProperties == null ? ImmutableMap.of() : catalogProperties;
}
+ public void close() {
+ fileIO.close();
+ clients.close();
Review Comment:
So when I have a `HiveCatalog`, I can just throw it away? This seems strange with resources inside it that seemingly need to have `close`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] NeQuissimus commented on a diff in pull request #5166: HiveCatalog: Close resources underlying various classes
Posted by GitBox <gi...@apache.org>.
NeQuissimus commented on code in PR #5166:
URL: https://github.com/apache/iceberg/pull/5166#discussion_r911090043
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -547,6 +547,11 @@ protected Map<String, String> properties() {
return catalogProperties == null ? ImmutableMap.of() : catalogProperties;
}
+ public void close() {
+ fileIO.close();
+ clients.close();
Review Comment:
This is why I did not link this method into existing code paths and left it completely optional.
For users of `HiveCatalog` there needs to be a way to close the resources.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5166: HiveCatalog: Close resources underlying various classes
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5166:
URL: https://github.com/apache/iceberg/pull/5166#discussion_r911624341
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -547,6 +547,11 @@ protected Map<String, String> properties() {
return catalogProperties == null ? ImmutableMap.of() : catalogProperties;
}
+ public void close() {
+ fileIO.close();
+ clients.close();
Review Comment:
Took some time to wrap my head around this, but that's the current situation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5166: HiveCatalog: Close resources underlying various classes
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5166:
URL: https://github.com/apache/iceberg/pull/5166#discussion_r911096792
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -547,6 +547,11 @@ protected Map<String, String> properties() {
return catalogProperties == null ? ImmutableMap.of() : catalogProperties;
}
+ public void close() {
+ fileIO.close();
+ clients.close();
Review Comment:
Based on the timeout the pool should be closed by inactivity:
See:
```
clientPoolCache = Caffeine.newBuilder().expireAfterAccess(evictionInterval, TimeUnit.MILLISECONDS)
.removalListener((key, value, cause) -> ((HiveClientPool) value).close())
.build();
```
I am afraid that having a close method will communicate the users that this should be called every time, which will cause performance issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org