NealSun96 commented on code in PR #2255:
URL: https://github.com/apache/helix/pull/2255#discussion_r1004811379
##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/ZkTestBase.java:
##########
@@ -124,7 +123,7 @@ private void setupZooKeepers() {
* @param zkAddress
* @return
*/
- protected ZkServer startZkServer(final String zkAddress) {
+ protected synchronized static ZkServer startZkServer(final String zkAddress)
{
Review Comment:
Why does this need to be synchronized? It doesn't look like the function
body involves any concurrent parts, unless ZkServer starting doesn't work
concurrently on the same dir?
##########
zookeeper-api/src/test/java/org/apache/helix/zookeeper/impl/client/TestSharedZkClient.java:
##########
@@ -40,7 +40,7 @@ public void beforeClass() throws IOException,
InvalidRoutingDataException {
_realmAwareZkClientFactory = SharedZkClientFactory.getInstance();
}
- @Test
+ @Test(dependsOnMethods = "testRealmAwareZkClientCreation")
Review Comment:
Is there any point on keeping this test case at a different class? If it
depends on _realmAwareZkClient that has to be created, then maybe this should
just go to RealmAwareZkClientFactoryTestBase since that's what this is testing.
##########
helix-core/src/test/java/org/apache/helix/integration/multizk/TestMultiZkConectionConfig.java:
##########
@@ -225,18 +220,6 @@ public void afterClass() throws Exception {
System.clearProperty(MetadataStoreRoutingConstants.MSDS_SERVER_ENDPOINT_KEY);
}
}
-
- boolean status = false;
- try {
- status = ThreadLeakageChecker.afterClassCheck(testClassName);
- } catch (Exception e) {
- LOG.error("ThreadLeakageChecker exception:", e);
- }
- // todo: We should fail test here once we achieved 0 leakage and remove
the following System print
- if (!status) {
- System.out.println(
- "---------- Test Class " + testClassName + " thread leakage
detected! ---------------");
- }
Review Comment:
About thread leakage: back in 2020 there was a huge effort to crack down
thread leak, and I believe those code is a part of that effort. Now that we're
removing it, how do we ensure that there's no thread leakage? What's the
strategy here?
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
