You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/11/17 18:39:08 UTC
[GitHub] [helix] jiajunwang commented on a change in pull request #1534: Fix several tests with resource leakage
jiajunwang commented on a change in pull request #1534:
URL: https://github.com/apache/helix/pull/1534#discussion_r525394604
##########
File path: helix-core/src/test/java/org/apache/helix/participant/TestDistControllerElection.java
##########
@@ -72,36 +73,45 @@ public void testController() throws Exception {
DistributedLeaderElection election =
new DistributedLeaderElection(manager, controller0, timerTasks);
NotificationContext context = new NotificationContext(manager);
- context.setType(NotificationContext.Type.INIT);
- election.onControllerChange(context);
-
- // path = PropertyPathConfig.getPath(PropertyType.LEADER, clusterName);
- // ZNRecord leaderRecord = _gZkClient.<ZNRecord> readData(path);
- LiveInstance liveInstance = accessor.getProperty(keyBuilder.controllerLeader());
- AssertJUnit.assertEquals(controllerName, liveInstance.getInstanceName());
- // AssertJUnit.assertNotNull(election.getController());
- // AssertJUnit.assertNull(election.getLeader());
+ try {
+ context.setType(NotificationContext.Type.INIT);
+ election.onControllerChange(context);
+
+ // path = PropertyPathConfig.getPath(PropertyType.LEADER, clusterName);
+ // ZNRecord leaderRecord = _gZkClient.<ZNRecord> readData(path);
+ LiveInstance liveInstance = accessor.getProperty(keyBuilder.controllerLeader());
+ AssertJUnit.assertEquals(controllerName, liveInstance.getInstanceName());
+ // AssertJUnit.assertNotNull(election.getController());
+ // AssertJUnit.assertNull(election.getLeader());
+ } finally {
+ manager.disconnect();
+ controller0.shutdown();
Review comment:
nit, shutdown the controller first would be safer.
##########
File path: helix-core/src/test/java/org/apache/helix/participant/TestDistControllerElection.java
##########
@@ -72,36 +73,45 @@ public void testController() throws Exception {
DistributedLeaderElection election =
new DistributedLeaderElection(manager, controller0, timerTasks);
NotificationContext context = new NotificationContext(manager);
- context.setType(NotificationContext.Type.INIT);
- election.onControllerChange(context);
-
- // path = PropertyPathConfig.getPath(PropertyType.LEADER, clusterName);
- // ZNRecord leaderRecord = _gZkClient.<ZNRecord> readData(path);
- LiveInstance liveInstance = accessor.getProperty(keyBuilder.controllerLeader());
- AssertJUnit.assertEquals(controllerName, liveInstance.getInstanceName());
- // AssertJUnit.assertNotNull(election.getController());
- // AssertJUnit.assertNull(election.getLeader());
+ try {
+ context.setType(NotificationContext.Type.INIT);
+ election.onControllerChange(context);
+
+ // path = PropertyPathConfig.getPath(PropertyType.LEADER, clusterName);
+ // ZNRecord leaderRecord = _gZkClient.<ZNRecord> readData(path);
+ LiveInstance liveInstance = accessor.getProperty(keyBuilder.controllerLeader());
+ AssertJUnit.assertEquals(controllerName, liveInstance.getInstanceName());
+ // AssertJUnit.assertNotNull(election.getController());
+ // AssertJUnit.assertNull(election.getLeader());
+ } finally {
+ manager.disconnect();
+ controller0.shutdown();
+ }
manager =
new MockZKHelixManager(clusterName, "controller_1", InstanceType.CONTROLLER, _gZkClient);
GenericHelixController controller1 = new GenericHelixController();
election = new DistributedLeaderElection(manager, controller1, timerTasks);
context = new NotificationContext(manager);
context.setType(NotificationContext.Type.INIT);
- election.onControllerChange(context);
- // leaderRecord = _gZkClient.<ZNRecord> readData(path);
- liveInstance = accessor.getProperty(keyBuilder.controllerLeader());
- AssertJUnit.assertEquals(controllerName, liveInstance.getInstanceName());
- // AssertJUnit.assertNull(election.getController());
- // AssertJUnit.assertNull(election.getLeader());
-
- accessor.removeProperty(keyBuilder.controllerLeader());
- TestHelper.dropCluster(clusterName, _gZkClient);
+ try {
+ election.onControllerChange(context);
+ // leaderRecord = _gZkClient.<ZNRecord> readData(path);
+ LiveInstance liveInstance = accessor.getProperty(keyBuilder.controllerLeader());
+ AssertJUnit.assertEquals(controllerName, liveInstance.getInstanceName());
+ // AssertJUnit.assertNull(election.getController());
+ // AssertJUnit.assertNull(election.getLeader());
+ } finally {
+ manager.disconnect();
+ controller1.shutdown();
Review comment:
Same here.
##########
File path: helix-core/src/test/java/org/apache/helix/tools/TestClusterSetup.java
##########
@@ -73,44 +73,63 @@
public void beforeClass() throws Exception {
System.out
.println("START TestClusterSetup.beforeClass() " + new Date(System.currentTimeMillis()));
+ _clusterSetup = new ClusterSetup(ZK_ADDR);
}
@AfterClass()
public void afterClass() {
deleteCluster(CLUSTER_NAME);
+ _clusterSetup.close();
System.out.println("END TestClusterSetup.afterClass() " + new Date(System.currentTimeMillis()));
}
@BeforeMethod()
public void setup() {
-
- _gZkClient.deleteRecursively("/" + CLUSTER_NAME);
- _clusterSetup = new ClusterSetup(ZK_ADDR);
- _clusterSetup.addCluster(CLUSTER_NAME, true);
+ try {
+ _gZkClient.deleteRecursively("/" + CLUSTER_NAME);
+ _clusterSetup.addCluster(CLUSTER_NAME, true);
+ } catch (Exception e) {
+ System.out.println("@BeforeMethod TestClusterSetup exception:" + e);
+ }
}
- @Test
+ // Note, with mvn 3.6.1, we have a nasty bug that running "mvn test" under helix-core,
+ // all the bellow test will be invoked after other test including @AfterClass cleanup of this
+ // This bug does not happen of running command as "mvn test -Dtest=TestClusterSetup". Nor does it
+ // happen in intellij. The workaround found is to add dependsOnMethods attribute to all the rest.
+ @Test(dependsOnMethods = "testAddClusterWithValidCloudConfig")
public void testZkAdminTimeout() {
boolean exceptionThrown = false;
+ ZKHelixAdmin admin = null;
Review comment:
nit, you probably want to extract a private method for this section to reduce duplication.
##########
File path: helix-core/src/test/java/org/apache/helix/participant/TestDistControllerElection.java
##########
@@ -197,35 +218,42 @@ public void testCompeteLeadership() throws Exception {
// Create controller leaders
final Map<String, ZKHelixManager> managerList = new HashMap<>();
+ final List<GenericHelixController> controllers = new ArrayList<>();
for (int i = 0; i < managerCount; i++) {
String controllerName = "controller_" + i;
ZKHelixManager manager =
new ZKHelixManager(clusterName, controllerName, InstanceType.CONTROLLER, ZK_ADDR);
GenericHelixController controller0 = new GenericHelixController();
DistributedLeaderElection election =
new DistributedLeaderElection(manager, controller0, Collections.EMPTY_LIST);
+ controllers.add(controller0);
manager.connect();
managerList.put(manager.getInstanceName(), manager);
}
- // Remove leader manager one by one, and verify if the leader node exists
- while(!managerList.isEmpty()) {
- // Ensure a controller successfully acquired leadership.
- Assert.assertTrue(TestHelper.verify(new TestHelper.Verifier() {
- @Override
- public boolean verify() {
- LiveInstance liveInstance = accessor.getProperty(keyBuilder.controllerLeader());
- if (liveInstance != null) {
- // disconnect the current leader manager
- managerList.remove(liveInstance.getInstanceName()).disconnect();
- return true;
- } else {
- return false;
+ try {
+ // Remove leader manager one by one, and verify if the leader node exists
+ while (!managerList.isEmpty()) {
+ // Ensure a controller successfully acquired leadership.
+ Assert.assertTrue(TestHelper.verify(new TestHelper.Verifier() {
+ @Override
+ public boolean verify() {
+ LiveInstance liveInstance = accessor.getProperty(keyBuilder.controllerLeader());
+ if (liveInstance != null) {
+ // disconnect the current leader manager
+ managerList.remove(liveInstance.getInstanceName()).disconnect();
+ return true;
+ } else {
+ return false;
+ }
}
- }
- }, 1000));
+ }, 1000));
+ }
+ } finally {
+ for (GenericHelixController controller : controllers) {
+ controller.shutdown();
Review comment:
How about the managers? Please disconnect them too.
----------------------------------------------------------------
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