[GitHub] [helix] jiajunwang commented on a change in pull request #1185: Fix ZkHelixClusterVerifier related resource leakage

[GitBox <gi...@apache.org>]

jiajunwang commented on a change in pull request #1185:
URL: https://github.com/apache/helix/pull/1185#discussion_r465418317



##########
File path: helix-core/src/test/java/org/apache/helix/integration/TestDriver.java
##########
@@ -246,7 +246,11 @@ public static void verifyCluster(String uniqClusterName, long beginTime, long ti
 
     ZkHelixClusterVerifier verifier =
         new BestPossibleExternalViewVerifier.Builder(clusterName).setZkAddr(ZK_ADDR).build();

Review comment:
       Just curious, why somewhere we use _gZkCLient, but other places we keep using the ZK_ADDR?
   
   If this is because of the test creates more ZKServers, then will the people who add more verifiers with the raw address make more leakage later?

##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -215,13 +218,21 @@ protected ZkClient(IZkConnection zkConnection, int connectionTimeout, long opera
       throw new NullPointerException("Zookeeper connection is null!");
     }
 
+    _uid = UID.getAndIncrement();
+    if (LOG.isInfoEnabled()) {
+      LOG.info("ZkClient created with _uid {}, stacktrace {}", _uid, Thread.currentThread().getStackTrace());

Review comment:
       This is not testing code, please be careful not to pollute the log in production. I think recording this info in the related thread would be good enough to support detecting leakage in test.

##########
File path: helix-lock/src/test/java/org/apache/helix/lock/helix/TestZKHelixNonblockingLock.java
##########
@@ -76,6 +76,7 @@ public void beforeMethod() {
 
   @AfterSuite
   public void afterSuite() throws IOException {
+    System.out.println("afterSuite " + getClass().getName());

Review comment:
       Same here, what's the usage of this output?

##########
File path: helix-core/src/test/java/org/apache/helix/integration/manager/ClusterManager.java
##########
@@ -74,7 +81,8 @@ public void syncStart() {
 
     _watcher = new Thread(this);
     _watcher.setName(String
-        .format("ClusterManager_Watcher_%s_%s_%s", _clusterName, _instanceName, _type.name()));
+        .format("ClusterManager_Watcher_%s_%s_%s_%d", _clusterName, _instanceName, _type.name(), _uid));
+    LOG.info("ClusterManager_watcher_{}_{}_{}_{} started, stacktrace {}", _clusterName, _instanceName, _type.name(), _uid, Thread.currentThread().getStackTrace());

Review comment:
       What's the usage of stacktrace? If this is for human debug, I would suggest making it debug log.

##########
File path: helix-core/src/test/java/org/apache/helix/common/ZkTestBase.java
##########
@@ -200,6 +200,7 @@ private void startZooKeeper(int i)
 
   @AfterSuite
   public void afterSuite() throws IOException {
+    System.out.println("afterSuite " + getClass().getName());

Review comment:
       AfterSuite is done after all test classes have been executed. So if you attach the class name, it won't be very informative, IMHO. What's the usage of this log?

##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -215,13 +218,21 @@ protected ZkClient(IZkConnection zkConnection, int connectionTimeout, long opera
       throw new NullPointerException("Zookeeper connection is null!");
     }
 
+    _uid = UID.getAndIncrement();
+    if (LOG.isInfoEnabled()) {
+      LOG.info("ZkClient created with _uid {}, stacktrace {}", _uid, Thread.currentThread().getStackTrace());
+    }
+
     _connection = zkConnection;
     _pathBasedZkSerializer = zkSerializer;
     _operationRetryTimeoutInMillis = operationRetryTimeout;
     _isNewSessionEventFired = false;
 
     _asyncCallRetryThread = new ZkAsyncRetryThread(zkConnection.getServers());
     _asyncCallRetryThread.start();
+    if (LOG.isInfoEnabled()) {
+      LOG.info("ZkClient created with _uid {}, _asyncCallRetryThread id {}", _uid, _asyncCallRetryThread.getId());

Review comment:
       Same here

##########
File path: helix-core/src/test/java/org/apache/helix/integration/manager/ClusterManager.java
##########
@@ -51,6 +55,9 @@ protected ClusterManager(String zkAddr, String clusterName, String instanceName,
     _clusterName = clusterName;
     _instanceName = instanceName;
     _type = type;
+    _uid = UID.getAndIncrement();
+
+    LOG.info("ClusterManager_watcher_{}_{}_{}_{} created, stacktrace {}", _clusterName, _instanceName, _type.name(), _uid, Thread.currentThread().getStackTrace());

Review comment:
       I don't think watcher is created here. It was created when sync start is called.

##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -2157,6 +2166,8 @@ public void connect(final long maxMsToWaitUntilConnected, Watcher watcher)
       _eventThread = new ZkEventThread(zkConnection.getServers());
       _eventThread.start();
 
+      LOG.info("ZkClient created with _uid {}, _eventThread {}", _uid, _eventThread.getId());

Review comment:
       Same here

##########
File path: helix-core/src/test/resources/log4j.properties
##########
@@ -17,7 +17,7 @@
 # under the License.
 #
 # Set root logger level to DEBUG and its only appender to R.
-log4j.rootLogger=ERROR, C
+log4j.rootLogger=ERROR, C, R

Review comment:
       Do we need this by default? If this is just for human investigation, please don't enable it.
   
   IMO, the only condition we need the file is when we have a bot exam the log file and print some critical information automatically.

##########
File path: helix-core/src/test/resources/log4j.properties
##########
@@ -37,5 +38,6 @@ log4j.logger.org.I0Itec=ERROR
 log4j.logger.org.apache=ERROR
 log4j.logger.com.noelios=ERROR
 log4j.logger.org.restlet=ERROR
+#log4j.logger.org.apache.helix=INFO

Review comment:
       Remove




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org