fapaul commented on a change in pull request #15707:
URL: https://github.com/apache/flink/pull/15707#discussion_r618256544
##########
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/RetryRule.java
##########
@@ -25,19 +25,24 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import javax.annotation.Nullable;
+
/**
* A rule to retry failed tests for a fixed number of times.
*
- * <p>Add the {@link RetryRule} to your test and annotate tests with {@link
RetryOnFailure}.
+ * <p>Add the {@link RetryRule} to your test class and annotate the class
and/or tests with {@link
+ * RetryOnFailure} or {@link RetryOnException}. If both the class and test is
annotated, then only
+ * the latter annotation is taken into account.
*
* <pre>
+ * {@literal @}RetryOnFailure(times=1)
* public class YourTest {
*
* {@literal @}Rule
* public RetryRule retryRule = new RetryRule();
*
* {@literal @}Test
- * {@literal @}RetryOnFailure(times=1)
+ * {@literal @}RetryOnFailure(times=2)
Review comment:
Why did you change this line? It invalidates the comment below because
it will be retried 2 times and the total runs are 43.
##########
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/RetryRule.java
##########
@@ -25,19 +25,24 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import javax.annotation.Nullable;
+
/**
* A rule to retry failed tests for a fixed number of times.
*
- * <p>Add the {@link RetryRule} to your test and annotate tests with {@link
RetryOnFailure}.
+ * <p>Add the {@link RetryRule} to your test class and annotate the class
and/or tests with {@link
Review comment:
Nit: To not surprise people when using it with both annotations.
```suggestion
* <p>Add the {@link RetryRule} to your test class and annotate the class
and/or tests with either {@link
```
##########
File path:
flink-connectors/flink-connector-cassandra/src/test/java/org/apache/flink/streaming/connectors/cassandra/CassandraConnectorITCase.java
##########
@@ -92,13 +96,16 @@
/** IT cases for all cassandra sinks. */
@SuppressWarnings("serial")
@Category(FailsOnJava11.class)
+@RetryOnException(times = 2, exception = NoHostAvailableException.class)
Review comment:
Nit: Add a small comment why it is necessary to give some visibility
what problem you are trying to solve.
##########
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/RetryRule.java
##########
@@ -70,26 +71,45 @@ public Statement apply(Statement statement, Description
description) {
"You cannot combine the RetryOnFailure and
RetryOnException annotations.");
}
+ final Class<? extends Throwable> whitelistedException =
getExpectedException(description);
+
if (retryOnFailure != null) {
- return new RetryOnFailureStatement(retryOnFailure.times(),
statement);
+ return new RetryOnFailureStatement(
+ retryOnFailure.times(), statement, whitelistedException);
Review comment:
Hmm, I do not understand why `RetryOnFailureStatement` has
`whiteListedException` as a parameter. What would be a benefit to fail early on
certain exceptions?
##########
File path:
flink-test-utils-parent/flink-test-utils-junit/src/main/java/org/apache/flink/testutils/junit/RetryRule.java
##########
@@ -70,26 +71,45 @@ public Statement apply(Statement statement, Description
description) {
"You cannot combine the RetryOnFailure and
RetryOnException annotations.");
}
+ final Class<? extends Throwable> whitelistedException =
getExpectedException(description);
+
if (retryOnFailure != null) {
- return new RetryOnFailureStatement(retryOnFailure.times(),
statement);
+ return new RetryOnFailureStatement(
+ retryOnFailure.times(), statement, whitelistedException);
Review comment:
I would expect that `RetryOnFailureStatement` only handles assertion
exceptions and throws all others if not it seems very similar to
`RetryOnException`
--
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]
