jiajunwang commented on a change in pull request #1292:
URL: https://github.com/apache/helix/pull/1292#discussion_r483234049
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/ZkConnectionManager.java
##########
@@ -48,12 +47,12 @@
* While multiple Shared ZkClients can use single connection manager if
possible.
*/
public class ZkConnectionManager extends ZkClient {
- private static Logger LOG =
LoggerFactory.getLogger(ZkConnectionManager.class);
// Client type that is used in monitor, and metrics.
private final static String MONITOR_TYPE = "ZkConnectionManager";
- private final String _monitorKey;
+ private static Logger LOG =
LoggerFactory.getLogger(ZkConnectionManager.class);
Review comment:
nit, please avoid unnecessary changes.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/SharedZkClientFactory.java
##########
@@ -117,6 +117,9 @@ private ZkConnectionManager getOrCreateZkConnectionManager(
private void cleanupConnectionManager(ZkConnectionManager
zkConnectionManager) {
synchronized (_connectionManagerPool) {
zkConnectionManager.close(true);
+ if (!zkConnectionManager.hasSharedWatchers()) {
Review comment:
This shall work, but will it be easier if we let close() return a
boolean to indicate if the real close has been done? If true, then it is safe
to remove it from the pool.
It is both easier and safer because once a zkConnectionManager is closed,
there is no way to reuse it.
So even if later we modify the concurrency control, there is no risk at all.
##########
File path:
zookeeper-api/src/main/java/org/apache/helix/zookeeper/impl/factory/ZkConnectionManager.java
##########
@@ -132,6 +131,10 @@ protected synchronized void close(boolean skipIfWatched) {
LOG.info("ZkConnection {} was closed.", _monitorKey);
}
+ protected boolean hasSharedWatchers() {
Review comment:
Please change the other logic that doing the same check refers to this
method.
##########
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:
Please finish the test : )
----------------------------------------------------------------
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]
