jiajunwang commented on a change in pull request #1292:
URL: https://github.com/apache/helix/pull/1292#discussion_r488402272
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##########
@@ -116,7 +116,10 @@ private ZkConnectionManager getOrCreateZkConnectionManager(
// to avoid race condition.
private void cleanupConnectionManager(ZkConnectionManager
zkConnectionManager) {
synchronized (_connectionManagerPool) {
- zkConnectionManager.close(true);
+ boolean canClose = zkConnectionManager.close(true);
Review comment:
nit, canClose => connectionClosed
Or maybe simpler,
if (zkConnectionManager.close(true)) {
_connectionManagerPool.remove(zkConnectionManager);
}
##########
File path:
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestSharedZkClient.java
##########
@@ -66,4 +65,16 @@ public void testCreateEphemeralFailure() {
// this is expected.
}
}
+
+ /*
+ Tests clean up of closed connection from pool
+ Use cases tested
+ -Not removal of connection when there is a active watchers
+ -Removal of connection when there are no active watchers
+ */
+ @Test
+ public void testCleanupOfClosedConnectionFromConnectionManager(){
Review comment:
One way that I can think of is modifying the test only method
getActiveConnectionCount() so it can return either the total connection manager
count or active manager count according to an input parameter.
My only concern is that I don't like the idea of making these test methods
more complicated : ( However, it might be the easiest way.
Please try to think of any better idea. If we really don't have anything
better, upgrading getActiveConnectionCount is still an OK option.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
