slachiewicz commented on code in PR #288:
URL:
https://github.com/apache/flink-connector-kafka/pull/288#discussion_r4504555646
##########
flink-connector-kafka/pom.xml:
##########
@@ -418,6 +418,24 @@ under the License.
</execution>
</executions>
</plugin>
+
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <properties>
+ <!-- Fail a hung test method
after 15 minutes, with its stack trace, instead of
+ letting it consume the
CI job's timeout. A test can override this with @Timeout.
+ The test body runs in a
separate thread so that a method which swallows the
+ interrupt still fails
on time and the rest of the suite runs. -->
+ <configurationParameters>
Review Comment:
You are right: the shared CI workflow's failure step already prints the
surviving JVMs' threads. The PR body and commit message now say what the
timeout adds instead: the hung method is named and failed, the dump is taken
just before the interrupt, and the rest of the suite reports.
##########
flink-connector-kafka/pom.xml:
##########
@@ -418,6 +418,24 @@ under the License.
</execution>
</executions>
</plugin>
+
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <properties>
+ <!-- Fail a hung test method
after 15 minutes, with its stack trace, instead of
+ letting it consume the
CI job's timeout. A test can override this with @Timeout.
+ The test body runs in a
separate thread so that a method which swallows the
+ interrupt still fails
on time and the rest of the suite runs. -->
+ <configurationParameters>
+
junit.jupiter.execution.timeout.testable.method.default = 15 m
+
junit.jupiter.execution.timeout.thread.mode.default = SEPARATE_THREAD
+
junit.jupiter.execution.timeout.mode = disabled_on_debug
+ </configurationParameters>
Review Comment:
Added `junit.jupiter.execution.timeout.threaddump.enabled = true` to the
surefire parameters (5.12 and later; the managed JUnit is 5.13.3). A scratch
test with a 3-second `@Timeout` printed the dump of all threads before the
interrupt.
##########
flink-connector-kafka/pom.xml:
##########
@@ -418,6 +418,24 @@ under the License.
</execution>
</executions>
</plugin>
+
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-surefire-plugin</artifactId>
+ <configuration>
+ <properties>
+ <!-- Fail a hung test method
after 15 minutes, with its stack trace, instead of
+ letting it consume the
CI job's timeout. A test can override this with @Timeout.
+ The test body runs in a
separate thread so that a method which swallows the
+ interrupt still fails
on time and the rest of the suite runs. -->
+ <configurationParameters>
+
junit.jupiter.execution.timeout.testable.method.default = 15 m
Review Comment:
The scope is test methods only, and the PR body and the pom comment now say
so, with the CI step timeout as the bound for setup and teardown hangs. I did
not set `junit.jupiter.execution.timeout.lifecycle.method.default` because the
thread mode default is shared: with it, `@BeforeEach` and the test ran on
`junit-timeout-thread-1` and `junit-timeout-thread-2`, which reintroduces the
thread-local problem `KafkaTableTestBase` works around.
##########
flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/table/KafkaTableTestBase.java:
##########
@@ -53,10 +54,18 @@
import java.util.Properties;
import java.util.Timer;
import java.util.TimerTask;
+import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
-/** Base class for Kafka Table IT Cases. */
+/**
+ * Base class for Kafka Table IT Cases.
+ *
+ * <p>Tests run on the thread that ran {@code @BeforeEach}, unlike the
module's default of a
+ * separate thread: the table planner leaves Calcite's metadata handler
provider in a thread-local
+ * there.
+ */
@Testcontainers
+@Timeout(value = 15, unit = TimeUnit.MINUTES, threadMode =
Timeout.ThreadMode.SAME_THREAD)
Review Comment:
The class Javadoc now says the 15 minutes repeat
`junit.jupiter.execution.timeout.testable.method.default` in the module pom and
must stay in step, and the pom comment points back here.
##########
flink-connector-kafka/src/test/java/org/apache/flink/streaming/connectors/kafka/table/KafkaTableTestBase.java:
##########
@@ -53,10 +54,18 @@
import java.util.Properties;
import java.util.Timer;
import java.util.TimerTask;
+import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
-/** Base class for Kafka Table IT Cases. */
+/**
+ * Base class for Kafka Table IT Cases.
+ *
+ * <p>Tests run on the thread that ran {@code @BeforeEach}, unlike the
module's default of a
+ * separate thread: the table planner leaves Calcite's metadata handler
provider in a thread-local
+ * there.
+ */
@Testcontainers
+@Timeout(value = 15, unit = TimeUnit.MINUTES, threadMode =
Timeout.ThreadMode.SAME_THREAD)
Review Comment:
It was intentional, and the PR body now says why: the SQL connector and
end-to-end modules run their tests in the `run-end-to-end-tests` profile
against a Flink distribution, and none of their method durations have been
measured against 15 minutes. I did not add the parameters there without being
able to run them; that fits a follow-up with measured times.
--
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]
