XComp commented on a change in pull request #19116:
URL: https://github.com/apache/flink/pull/19116#discussion_r832319562
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/leaderretrieval/DefaultLeaderRetrievalServiceTest.java
##########
@@ -112,13 +110,6 @@ public void testErrorIsIgnoredAfterBeingStop() throws
Exception {
leaderRetrievalService.stop();
testingLeaderRetrievalDriver.onFatalError(testException);
- try {
- testingListener.waitForError(timeout);
Review comment:
We cannot just remove this because waitForError polls the error from the
errorQueue in TestingRetrievalBase. This test doesn't even fail when
`leaderRetrievalService.stop()` is commented out.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/DefaultLeaderElectionServiceTest.java
##########
@@ -258,15 +256,6 @@ public void testErrorIsIgnoredAfterBeingStop() throws
Exception {
leaderElectionService.stop();
testingLeaderElectionDriver.onFatalError(testException);
-
- try {
- testingContender.waitForError(timeout);
Review comment:
We cannot just remove this because `waitForError` polls the error from
the `errorQueue` in `TestingLeaderBase`. This test doesn't even fail when
`leaderElectionService.stop()` is commented out.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/TestingRetrievalBase.java
##########
@@ -68,28 +62,21 @@ public String waitForNewLeader(long timeout) throws
Exception {
public void waitForEmptyLeaderInformation(long timeout) throws Exception {
throwExceptionIfNotNull();
- final String errorMsg =
- "Listener was not notified about an empty leader within " +
timeout + "ms";
CommonTestUtils.waitUntilCondition(
() -> {
leader = leaderEventQueue.poll(timeout,
TimeUnit.MILLISECONDS);
return leader != null && leader.isEmpty();
- },
- Deadline.fromNow(Duration.ofMillis(timeout)),
- errorMsg);
+ });
oldAddress = null;
}
public void waitForError(long timeout) throws Exception {
- final String errorMsg = "Listener did not see an exception with " +
timeout + "ms";
CommonTestUtils.waitUntilCondition(
() -> {
error = errorQueue.poll(timeout, TimeUnit.MILLISECONDS);
return error != null;
- },
- Deadline.fromNow(Duration.ofMillis(timeout)),
- errorMsg);
+ });
Review comment:
Just checked if that also applies for `waitFor[New|Empty]Leader`. But
there we still need the `oldAddress` for verification.
##########
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNHighAvailabilityITCase.java
##########
@@ -262,8 +261,7 @@ private void waitForApplicationAttempt(final ApplicationId
applicationId, final
yarnClient.getApplicationReport(applicationId);
return
applicationReport.getCurrentApplicationAttemptId().getAttemptId()
>= attemptId;
- },
- Deadline.fromNow(TIMEOUT));
+ });
Review comment:
nit: TIMEOUT is only used in `waitForJobTermination`. We could move the
value of the variable into that method to improve code readability...
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/TestingLeaderBase.java
##########
@@ -44,44 +42,35 @@
public void waitForLeader(long timeout) throws Exception {
throwExceptionIfNotNull();
- final String errorMsg = "Contender was not elected as the leader
within " + timeout + "ms";
CommonTestUtils.waitUntilCondition(
() -> {
final LeaderInformation leader =
leaderEventQueue.poll(timeout,
TimeUnit.MILLISECONDS);
return leader != null && !leader.isEmpty();
- },
- Deadline.fromNow(Duration.ofMillis(timeout)),
- errorMsg);
+ });
isLeader = true;
}
public void waitForRevokeLeader(long timeout) throws Exception {
throwExceptionIfNotNull();
- final String errorMsg = "Contender was not revoked within " + timeout
+ "ms";
CommonTestUtils.waitUntilCondition(
() -> {
final LeaderInformation leader =
leaderEventQueue.poll(timeout,
TimeUnit.MILLISECONDS);
return leader != null && leader.isEmpty();
- },
- Deadline.fromNow(Duration.ofMillis(timeout)),
- errorMsg);
+ });
isLeader = false;
}
public void waitForError(long timeout) throws Exception {
- final String errorMsg = "Contender did not see an exception with " +
timeout + "ms";
CommonTestUtils.waitUntilCondition(
() -> {
error = errorQueue.poll(timeout, TimeUnit.MILLISECONDS);
return error != null;
- },
- Deadline.fromNow(Duration.ofMillis(timeout)),
- errorMsg);
+ });
Review comment:
I'm wondering whether errorQueue.poll(...) without the
`waitUntilCondition` method call would be sufficient enough. Returning the
error would make `getError` obsolete as well.
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/leaderelection/TestingRetrievalBase.java
##########
@@ -68,28 +62,21 @@ public String waitForNewLeader(long timeout) throws
Exception {
public void waitForEmptyLeaderInformation(long timeout) throws Exception {
throwExceptionIfNotNull();
- final String errorMsg =
- "Listener was not notified about an empty leader within " +
timeout + "ms";
CommonTestUtils.waitUntilCondition(
() -> {
leader = leaderEventQueue.poll(timeout,
TimeUnit.MILLISECONDS);
return leader != null && leader.isEmpty();
- },
- Deadline.fromNow(Duration.ofMillis(timeout)),
- errorMsg);
+ });
oldAddress = null;
}
public void waitForError(long timeout) throws Exception {
- final String errorMsg = "Listener did not see an exception with " +
timeout + "ms";
CommonTestUtils.waitUntilCondition(
() -> {
error = errorQueue.poll(timeout, TimeUnit.MILLISECONDS);
return error != null;
- },
- Deadline.fromNow(Duration.ofMillis(timeout)),
- errorMsg);
+ });
Review comment:
Same as for the `TestingLeaderBase.waitForError` method
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/testutils/CommonTestUtils.java
##########
@@ -131,40 +128,16 @@ public static void printLog4jDebugConfig(File file)
throws IOException {
}
}
- public static void waitUntilCondition(
- SupplierWithException<Boolean, Exception> condition, Deadline
timeout)
- throws Exception {
- waitUntilCondition(condition, timeout, RETRY_INTERVAL);
- }
-
- public static void waitUntilCondition(
- SupplierWithException<Boolean, Exception> condition,
- Deadline timeout,
- long retryIntervalMillis)
- throws Exception {
- waitUntilCondition(
- condition, timeout, retryIntervalMillis, "Condition was not
met in given timeout.");
- }
-
- public static void waitUntilCondition(
- SupplierWithException<Boolean, Exception> condition, Deadline
timeout, String errorMsg)
+ public static void waitUntilCondition(SupplierWithException<Boolean,
Exception> condition)
Review comment:
Is there a reason why we keep the redundancy?
`org.apache.flink.core.testutils.CommonTestUtils` is accessible from within
`flink-runtime` as well.
##########
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionCapacitySchedulerITCase.java
##########
@@ -390,16 +389,14 @@ private void submitJob(final String jobFileName) throws
IOException, Interrupted
private static void waitForTaskManagerRegistration(
final String host, final int port, final Duration waitDuration)
throws Exception {
Review comment:
```suggestion
final String host, final int port) throws Exception {
```
`waitDuration` is not used anymore
##########
File path:
flink-yarn-tests/src/test/java/org/apache/flink/yarn/YARNSessionFIFOITCase.java
##########
@@ -157,12 +156,7 @@ ApplicationId runDetachedModeTest(Map<String, String>
securityProperties) throws
// wait until two containers are running
LOG.info("Waiting until two containers are running");
CommonTestUtils.waitUntilCondition(
- () -> getRunningContainers() >= 2,
- Deadline.fromNow(timeout),
- testConditionIntervalInMillis,
- "We didn't reach the state of two containers running (instead:
"
- + getRunningContainers()
- + ")");
+ () -> getRunningContainers() >= 2,
testConditionIntervalInMillis);
Review comment:
the local `timeout` variable is unused now.
--
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]
