XComp commented on a change in pull request #15954:
URL: https://github.com/apache/flink/pull/15954#discussion_r635038298
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/highavailability/zookeeper/ZooKeeperHaServicesTest.java
##########
@@ -224,23 +224,26 @@ private void runCleanupTestWithJob(
final RunningJobsRegistry runningJobsRegistry =
zooKeeperHaServices.getRunningJobsRegistry();
- final LeaderRetrievalUtils.LeaderConnectionInfoListener listener =
+ final LeaderRetrievalUtils.LeaderConnectionInfoListener
resourceManagerLeaderListener =
new LeaderRetrievalUtils.LeaderConnectionInfoListener();
- resourceManagerLeaderRetriever.start(listener);
+
resourceManagerLeaderRetriever.start(resourceManagerLeaderListener);
resourceManagerLeaderElectionService.start(
new TestingContender("foobar",
resourceManagerLeaderElectionService));
LeaderElectionService jobManagerLeaderElectionService =
zooKeeperHaServices.getJobManagerLeaderElectionService(jobId);
jobManagerLeaderElectionService.start(
- new TestingContender("", jobManagerLeaderElectionService));
+ new TestingContender("barfoo",
jobManagerLeaderElectionService));
Review comment:
```suggestion
new TestingContender("unused-jobmanager-address",
jobManagerLeaderElectionService));
```
nit: I understand that you just followed the scheme of the other
`TestingContender`. But I usually find it better to make the purpose of a
certain String parameters visible if possible. That improves the readability of
the test code. Without an IDE, I wouldn't be able to see right away what this
parameter is used for if I'm not familiar with the `TestingContender` class.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/highavailability/zookeeper/ZooKeeperHaServicesTest.java
##########
@@ -224,23 +224,26 @@ private void runCleanupTestWithJob(
final RunningJobsRegistry runningJobsRegistry =
zooKeeperHaServices.getRunningJobsRegistry();
- final LeaderRetrievalUtils.LeaderConnectionInfoListener listener =
+ final LeaderRetrievalUtils.LeaderConnectionInfoListener
resourceManagerLeaderListener =
new LeaderRetrievalUtils.LeaderConnectionInfoListener();
- resourceManagerLeaderRetriever.start(listener);
+
resourceManagerLeaderRetriever.start(resourceManagerLeaderListener);
resourceManagerLeaderElectionService.start(
new TestingContender("foobar",
resourceManagerLeaderElectionService));
LeaderElectionService jobManagerLeaderElectionService =
Review comment:
nit: `final` is missing
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/highavailability/zookeeper/ZooKeeperHaServicesTest.java
##########
@@ -224,23 +224,26 @@ private void runCleanupTestWithJob(
final RunningJobsRegistry runningJobsRegistry =
zooKeeperHaServices.getRunningJobsRegistry();
- final LeaderRetrievalUtils.LeaderConnectionInfoListener listener =
+ final LeaderRetrievalUtils.LeaderConnectionInfoListener
resourceManagerLeaderListener =
new LeaderRetrievalUtils.LeaderConnectionInfoListener();
- resourceManagerLeaderRetriever.start(listener);
+
resourceManagerLeaderRetriever.start(resourceManagerLeaderListener);
resourceManagerLeaderElectionService.start(
new TestingContender("foobar",
resourceManagerLeaderElectionService));
LeaderElectionService jobManagerLeaderElectionService =
zooKeeperHaServices.getJobManagerLeaderElectionService(jobId);
jobManagerLeaderElectionService.start(
- new TestingContender("", jobManagerLeaderElectionService));
+ new TestingContender("barfoo",
jobManagerLeaderElectionService));
LeaderRetrievalService jobManagerLeaderRetriever =
zooKeeperHaServices.getJobManagerLeaderRetriever(jobId);
- jobManagerLeaderRetriever.start(
- new LeaderRetrievalUtils.LeaderConnectionInfoListener());
+ final LeaderRetrievalUtils.LeaderConnectionInfoListener
jobMasterLeaderListener =
Review comment:
nit: `jobManagerLeaderListener` might be a better name for consistency
purposes. But I understand that `jobMasterLeaderListener` is the more correct
name.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/highavailability/zookeeper/ZooKeeperHaServicesTest.java
##########
@@ -224,23 +224,26 @@ private void runCleanupTestWithJob(
final RunningJobsRegistry runningJobsRegistry =
zooKeeperHaServices.getRunningJobsRegistry();
- final LeaderRetrievalUtils.LeaderConnectionInfoListener listener =
+ final LeaderRetrievalUtils.LeaderConnectionInfoListener
resourceManagerLeaderListener =
new LeaderRetrievalUtils.LeaderConnectionInfoListener();
- resourceManagerLeaderRetriever.start(listener);
+
resourceManagerLeaderRetriever.start(resourceManagerLeaderListener);
resourceManagerLeaderElectionService.start(
new TestingContender("foobar",
resourceManagerLeaderElectionService));
LeaderElectionService jobManagerLeaderElectionService =
zooKeeperHaServices.getJobManagerLeaderElectionService(jobId);
jobManagerLeaderElectionService.start(
- new TestingContender("", jobManagerLeaderElectionService));
+ new TestingContender("barfoo",
jobManagerLeaderElectionService));
LeaderRetrievalService jobManagerLeaderRetriever =
Review comment:
nit: `final` is missing
--
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]
