Hive: CachedClientPool connection leak #2649
Conversation
| @@ -65,6 +65,8 @@ private synchronized void init() { | |||
| .removalListener((key, value, cause) -> ((HiveClientPool) value).close()) | |||
| .build(); | |||
| } | |||
| // cleanup cache before jvm exit, avoid some clients may not be closed. | |||
| Runtime.getRuntime().addShutdownHook(new Thread(CachedClientPool::cleanupCache)); | |||
There was a problem hiding this comment.
We should close the HiveClientPool first～
CachedClientPool.clientPoolCache().getIfPresent(metastoreUri).close();
CachedClientPool.clientPoolCache().invalidateAll();
There was a problem hiding this comment.
What is the downside of not closing the clients when the JVM exits? IMHO the lifetime of the client pool should be tied to the owning resources and be cleaned up through a Closeable interface rather than at system exit
There was a problem hiding this comment.
If a large number of tasks exit, many connections may not be closed. But as you said, this is not a serious problem.
| @@ -65,6 +65,8 @@ private synchronized void init() { | |||
| .removalListener((key, value, cause) -> ((HiveClientPool) value).close()) | |||
| .build(); | |||
| } | |||
| // cleanup cache before jvm exit, avoid some clients may not be closed. | |||
| Runtime.getRuntime().addShutdownHook(new Thread(CachedClientPool::cleanupCache)); | |||
There was a problem hiding this comment.
What is the downside of not closing the clients when the JVM exits? IMHO the lifetime of the client pool should be tied to the owning resources and be cleaned up through a Closeable interface rather than at system exit
| @@ -36,6 +38,10 @@ | |||
| private volatile int currentSize; | |||
| private boolean closed; | |||
|
|
|||
| private static volatile AtomicInteger openCount = new AtomicInteger(0); | |||
There was a problem hiding this comment.
I am not sure if I understand the purpose of these three variables
There was a problem hiding this comment.
This is just for counting the number of connections，maybe it can be used as a metric in the future.
There was a problem hiding this comment.
I'm a little confused on the variables as well.
If I'm not mistaken, it's the total number of connections ever opened and the total number of connections closed? Looking at the declaration, I would have thought that it was the current number of open connections, not the number of connections ever opened.
My only indication that it might be otherwise came from the unit test - https://github.com/apache/iceberg/pull/2649/files#diff-8b2035adec2ca6d247f624aeb6dbb2cc3789bd188541c500de4319b969813758R78-R80
private void waitingForCleanup() {
long startTime = System.currentTimeMillis();
while ((ClientPoolImpl.openCount().get() != ClientPoolImpl.closeCount().get()) Maybe you could add a small comment?
There was a problem hiding this comment.
Thanks for your time, yes your right, it's the total number of connections ever opened and the total number of connections closed，just for unit test, maybe the log is also useful.
As @southernriver suggested, closed directly would be better. it is no need to wait.
| @@ -36,6 +38,10 @@ | |||
| private volatile int currentSize; | |||
| private boolean closed; | |||
|
|
|||
| private static volatile AtomicInteger openCount = new AtomicInteger(0); | |||
There was a problem hiding this comment.
I'm a little confused on the variables as well.
If I'm not mistaken, it's the total number of connections ever opened and the total number of connections closed? Looking at the declaration, I would have thought that it was the current number of open connections, not the number of connections ever opened.
My only indication that it might be otherwise came from the unit test - https://github.com/apache/iceberg/pull/2649/files#diff-8b2035adec2ca6d247f624aeb6dbb2cc3789bd188541c500de4319b969813758R78-R80
private void waitingForCleanup() {
long startTime = System.currentTimeMillis();
while ((ClientPoolImpl.openCount().get() != ClientPoolImpl.closeCount().get()) Maybe you could add a small comment?
| public static AtomicInteger openCount() { | ||
| return openCount; | ||
| } | ||
|
|
||
| public static AtomicInteger closeCount() { | ||
| return closeCount; |
There was a problem hiding this comment.
Would it make more sense to call .get() in the return statement and just return an Integer?
It seems the only places in the code that these functions are called, .get() is called immediately. It seems to me that calling .get() would provide the benefit of a defensive copy to truly leave these private.
There was a problem hiding this comment.
Thanks，your suggestion is very helpful, i'll do it.
|
@rymurr @kbendick @southernriver thanks, i will close this pr. |
|
@StefanXiepj I reopened this and do some fix in another pr #2785 |
What changes were proposed in this pull request?
This pr adds shutdownhook to close the connection in the cache of CachedClientPool before the jvm exit.
Why are the changes needed?
ExpireAfterAccess is 5 minutes, this is too long, if the last access time is much less than 5 minutes, the connections in the cache will leak，we should close it.