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 2021/07/06 11:50:11 UTC
[GitHub] [iceberg] southernriver opened a new pull request #2785: Fix the problem that the metastore-client is not closed
southernriver opened a new pull request #2785:
URL: https://github.com/apache/iceberg/pull/2785
Here we add close() to prove that there is a memory leak in all unit test, another solution is that we need to add this close() into shutdownhook.
--
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] southernriver commented on a change in pull request #2785: Fix the problem that the metastore-client is not closed
Posted by GitBox <gi...@apache.org>.
southernriver commented on a change in pull request #2785:
URL: https://github.com/apache/iceberg/pull/2785#discussion_r665828962
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/CachedClientPool.java
##########
@@ -65,6 +68,14 @@ private synchronized void init() {
.removalListener((key, value, cause) -> ((HiveClientPool) value).close())
.build();
}
+ Runtime.getRuntime().addShutdownHook(new Thread() {
+ @Override
+ public void run() {
+ LOG.info("Before clientPool closed, clientPool().getCurrentSize() = {}", clientPool().getCurrentSize());
+ clientPool().close();
Review comment:
The clientPool() should never be null from the code :
```
HiveClientPool clientPool() {
return clientPoolCache.get(metastoreUri, k -> new HiveClientPool(clientPoolSize, conf));
}
```
And the active connection has been checked inside function of `close() `itself as follow:
https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/ClientPoolImpl.java#L86
--
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] southernriver commented on a change in pull request #2785: Fix the problem that the metastore-client is not closed
Posted by GitBox <gi...@apache.org>.
southernriver commented on a change in pull request #2785:
URL: https://github.com/apache/iceberg/pull/2785#discussion_r664980323
##########
File path: baseline.gradle
##########
@@ -79,6 +79,7 @@ subprojects {
'-Xep:StrictUnusedVariable:OFF',
'-Xep:TypeParameterShadowing:OFF',
'-Xep:TypeParameterUnusedInFormals:OFF',
+ '-Xep:ShutdownHook:OFF',
Review comment:
It just avoid building failure for gradle.
As https://github.com/palantir/gradle-baseline mentioned:
`ShutdownHook: Applications should not use Runtime#addShutdownHook.
`
and we can add this properties to skip this.
--
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] southernriver commented on a change in pull request #2785: Fix the problem that the metastore-client is not closed
Posted by GitBox <gi...@apache.org>.
southernriver commented on a change in pull request #2785:
URL: https://github.com/apache/iceberg/pull/2785#discussion_r664980323
##########
File path: baseline.gradle
##########
@@ -79,6 +79,7 @@ subprojects {
'-Xep:StrictUnusedVariable:OFF',
'-Xep:TypeParameterShadowing:OFF',
'-Xep:TypeParameterUnusedInFormals:OFF',
+ '-Xep:ShutdownHook:OFF',
Review comment:
It just avoid building failure for gradle.
As https://github.com/palantir/gradle-baseline mentioned:
`ShutdownHook: Applications should not use Runtime#addShutdownHook.
`
and we can add this property to skip this.
--
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] RussellSpitzer commented on a change in pull request #2785: Fix the problem that the metastore-client is not closed
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #2785:
URL: https://github.com/apache/iceberg/pull/2785#discussion_r664638358
##########
File path: baseline.gradle
##########
@@ -79,6 +79,7 @@ subprojects {
'-Xep:StrictUnusedVariable:OFF',
'-Xep:TypeParameterShadowing:OFF',
'-Xep:TypeParameterUnusedInFormals:OFF',
+ '-Xep:ShutdownHook:OFF',
Review comment:
Does this disable JVM Shutdown Hooks? I am a little confused that we are disabling this when we add a shutdown hook?
--
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 change in pull request #2785: Fix the problem that the metastore-client is not closed
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2785:
URL: https://github.com/apache/iceberg/pull/2785#discussion_r667895619
##########
File path: core/src/main/java/org/apache/iceberg/ClientPoolImpl.java
##########
@@ -140,4 +140,7 @@ private void release(C client) {
public int poolSize() {
return poolSize;
}
+ public int getCurrentSize() {
Review comment:
nit: newline
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/CachedClientPool.java
##########
@@ -65,6 +68,16 @@ private synchronized void init() {
.removalListener((key, value, cause) -> ((HiveClientPool) value).close())
.build();
}
+ Runtime.getRuntime().addShutdownHook(new Thread() {
Review comment:
nit: newline
--
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] kbendick commented on a change in pull request #2785: Fix the problem that the metastore-client is not closed
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #2785:
URL: https://github.com/apache/iceberg/pull/2785#discussion_r665759590
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/CachedClientPool.java
##########
@@ -65,6 +68,14 @@ private synchronized void init() {
.removalListener((key, value, cause) -> ((HiveClientPool) value).close())
.build();
}
+ Runtime.getRuntime().addShutdownHook(new Thread() {
+ @Override
+ public void run() {
+ LOG.info("Before clientPool closed, clientPool().getCurrentSize() = {}", clientPool().getCurrentSize());
Review comment:
Nit: Would this be better as a lower log level? Additionally, would it make more sense to format this as human readable / formatted text as opposed to references to functions (which might be awkward for end users)?
For example: `The CachedClientPool has a size of {}`.
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/CachedClientPool.java
##########
@@ -65,6 +68,14 @@ private synchronized void init() {
.removalListener((key, value, cause) -> ((HiveClientPool) value).close())
.build();
}
+ Runtime.getRuntime().addShutdownHook(new Thread() {
+ @Override
+ public void run() {
+ LOG.info("Before clientPool closed, clientPool().getCurrentSize() = {}", clientPool().getCurrentSize());
+ clientPool().close();
Review comment:
Do we need to check if `close` has already been called and/or if the result of calling `clientPool()` is not null?
--
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