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 01:46:31 UTC
[GitHub] [helix] kaisun2000 opened a new pull request #1534: Fix several tests with resource leakage
kaisun2000 opened a new pull request #1534:
URL: https://github.com/apache/helix/pull/1534
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
Fix several tests with resource leakage
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- [x] The following is the result of the "mvn test" command on the appropriate module:
github run pending
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang merged pull request #1534: Fix several tests with resource leakage
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1534:
URL: https://github.com/apache/helix/pull/1534
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1534: Fix several tests with resource leakage
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1534:
URL: https://github.com/apache/helix/pull/1534#discussion_r525622276
##########
File path: helix-core/src/test/java/org/apache/helix/controller/changedetector/TestResourceChangeDetector.java
##########
@@ -431,8 +431,12 @@ public void testResetSnapshots() {
.setResources(new HashSet<>(_dataAccessor.getChildNames(_keyBuilder.idealStates())))
.setWaitTillVerify(TestHelper.DEFAULT_REBALANCE_PROCESSING_WAIT_TIME)
.build();
- Assert.assertTrue(_clusterVerifier.verify());
-
+ try {
+ boolean result = _clusterVerifier.verify();
Review comment:
Changed.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on pull request #1534: Fix several tests with resource leakage
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1534:
URL: https://github.com/apache/helix/pull/1534#issuecomment-729304838
This PR is approved, please help to merge in.
> Fix several tests with resource leakage
----------------------------------------------------------------
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
[GitHub] [helix] jiajunwang commented on a change in pull request #1534: Fix several tests with resource leakage
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1534: Fix several tests with resource leakage
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1534:
URL: https://github.com/apache/helix/pull/1534#discussion_r525486813
##########
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:
good point. added.
##########
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:
changed.
##########
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:
changed.
----------------------------------------------------------------
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
[GitHub] [helix] kaisun2000 commented on a change in pull request #1534: Fix several tests with resource leakage
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on a change in pull request #1534:
URL: https://github.com/apache/helix/pull/1534#discussion_r525497997
##########
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:
added an test helper to reduce duplicated code.
----------------------------------------------------------------
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
[GitHub] [helix] xyuanlu commented on a change in pull request #1534: Fix several tests with resource leakage
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1534:
URL: https://github.com/apache/helix/pull/1534#discussion_r525607202
##########
File path: helix-core/src/test/java/org/apache/helix/controller/changedetector/TestResourceChangeDetector.java
##########
@@ -431,8 +431,12 @@ public void testResetSnapshots() {
.setResources(new HashSet<>(_dataAccessor.getChildNames(_keyBuilder.idealStates())))
.setWaitTillVerify(TestHelper.DEFAULT_REBALANCE_PROCESSING_WAIT_TIME)
.build();
- Assert.assertTrue(_clusterVerifier.verify());
-
+ try {
+ boolean result = _clusterVerifier.verify();
Review comment:
nit: Can we do
Assert.assertTrue(_clusterVerifier.verify()); ?
----------------------------------------------------------------
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