zentol commented on code in PR #22919:
URL: https://github.com/apache/flink/pull/22919#discussion_r1251054354
##########
flink-runtime/src/main/java/org/apache/flink/runtime/leaderelection/DefaultLeaderElectionService.java:
##########
@@ -268,9 +272,13 @@ public void close() throws Exception {
running = false;
} else {
LOG.debug("The HA backend connection isn't established. No
actions taken.");
+ return;
}
}
+ Preconditions.checkState(
+ leaderElectionDriver != null, "The HA backend wasn't
initialized.");
+
leaderElectionDriver.close();
Review Comment:
isnt it possible for a service to be closed without a contender ever being
registered (and thus the driver being null)?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/DefaultLeaderElectionServiceTest.java:
##########
@@ -180,25 +186,117 @@ void testCloseGrantDeadlock() throws Exception {
}
@Test
- void testGrantCallWhileInstantiatingDriver() throws Exception {
- final UUID expectedLeaderSessionID = UUID.randomUUID();
+ void testLazyDriverInstantiation() throws Exception {
+ final AtomicBoolean driverCreated = new AtomicBoolean();
try (final DefaultLeaderElectionService testInstance =
new DefaultLeaderElectionService(
(listener, errorHandler) -> {
- listener.isLeader(expectedLeaderSessionID);
+ driverCreated.set(true);
return TestingLeaderElectionDriver.newNoOpBuilder()
.build(listener, errorHandler);
},
fatalErrorHandlerExtension.getTestingFatalErrorHandler(),
Executors.newDirectExecutorService())) {
- testInstance.startLeaderElectionBackend();
+ assertThat(driverCreated)
+ .as("The driver shouldn't have been created during service
creation.")
+ .isFalse();
+
+ try (final LeaderElection leaderElection =
+ testInstance.createLeaderElection("contender-id")) {
+ assertThat(driverCreated)
+ .as(
+ "The driver shouldn't have been created during
LeaderElection creation.")
+ .isFalse();
+
+ leaderElection.startLeaderElection(
+ TestingGenericLeaderContender.newBuilder().build());
+ assertThat(driverCreated)
+ .as(
+ "The driver should have been created when
registering the contender in the LeaderElection.")
+ .isTrue();
+ }
+ }
+ }
+
+ @Test
+ void testReuseOfServiceIsRestricted() throws Exception {
+ final DefaultLeaderElectionService testInstance =
+ new DefaultLeaderElectionService(
+ new TestingLeaderElectionDriver.Factory(
+ TestingLeaderElectionDriver.newNoOpBuilder()));
+
+ // The driver hasn't started, yet, which prevents the service from
going into running state.
+ // This results in the close method not having any effect.
+ testInstance.close();
+
+ try (final LeaderElection leaderElection =
+ testInstance.createLeaderElection("contender-id")) {
Review Comment:
Wondering whether this shouldn't fail with an exception. Whether a driver
ended up being created or not shouldn't matter as to whether a closed LES can
still be used.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/DefaultLeaderElectionServiceTest.java:
##########
@@ -125,7 +126,12 @@ void testCloseGrantDeadlock() throws Exception {
final DefaultLeaderElectionService testInstance =
new DefaultLeaderElectionService(
driverFactory,
fatalErrorHandlerExtension.getTestingFatalErrorHandler());
- testInstance.startLeaderElectionBackend();
+
+ // creating the LeaderElection is necessary to instantiate the driver
+ final LeaderElection leaderElection =
testInstance.createLeaderElection("contender-id");
+
leaderElection.startLeaderElection(TestingGenericLeaderContender.newBuilder().build());
+ leaderElection.close();
Review Comment:
This seems sketchy. Given that the driver is lazily initialized once the
first contender is registered, wouldn't it be intuitive for the driver to be
closed when the last leader election is deregistered?
I know that's not the current behavior (maybe it should be? :thinking: ),
but basically I'm just saying that in case we change this later the leader
election shouldn't be closed until the end of the test.
--
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]
