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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
