wangyang0918 commented on a change in pull request #13968:
URL: https://github.com/apache/flink/pull/13968#discussion_r518771032
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/DefaultLeaderElectionServiceTest.java
##########
@@ -210,6 +212,33 @@ public void testErrorIsIgnoredAfterBeingStop() throws
Exception {
}};
}
+ /**
+ * Tests that we can shut down the DefaultLeaderElectionService if the
used
+ * LeaderElectionDriver holds an internal lock. See FLINK-20008 for
more details.
+ */
+ @Test
+ public void testServiceShutDownWithSynchronizedDriver() throws
Exception {
+ final
TestingLeaderElectionDriver.TestingLeaderElectionDriverFactory
testingLeaderElectionDriverFactory = new
TestingLeaderElectionDriver.TestingLeaderElectionDriverFactory();
+ final DefaultLeaderElectionService leaderElectionService = new
DefaultLeaderElectionService(
+ testingLeaderElectionDriverFactory);
+
+ final TestingContender testingContender = new
TestingContender(TEST_URL, leaderElectionService);
+
+ leaderElectionService.start(testingContender);
+ final TestingLeaderElectionDriver currentLeaderDriver =
Preconditions.checkNotNull(testingLeaderElectionDriverFactory.getCurrentLeaderDriver());
Review comment:
nit: maybe break a new line.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/DefaultLeaderElectionServiceTest.java
##########
@@ -210,6 +212,33 @@ public void testErrorIsIgnoredAfterBeingStop() throws
Exception {
}};
}
+ /**
+ * Tests that we can shut down the DefaultLeaderElectionService if the
used
+ * LeaderElectionDriver holds an internal lock. See FLINK-20008 for
more details.
+ */
+ @Test
Review comment:
I think this test could also pass in most cases without moving
`leaderElectionDriver.close()` out of `lock`. But it will fail in some corner
situation. Right?
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/leaderelection/DefaultLeaderElectionService.java
##########
@@ -292,13 +287,13 @@ public void onFatalError(Throwable throwable) {
}
return;
}
- }
- // The contender callback should be executed out of
lock to avoid potential deadlock.
- if (throwable instanceof LeaderElectionException) {
-
leaderContender.handleError((LeaderElectionException) throwable);
- } else {
- leaderContender.handleError(new
LeaderElectionException(throwable));
+ // The contender callback should be executed
out of lock to avoid potential deadlock.
Review comment:
The comment now is outdated.
----------------------------------------------------------------
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]
