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