Savonitar commented on code in PR #288:
URL:
https://github.com/apache/flink-connector-kafka/pull/288#discussion_r4159374517
##########
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:
This setting covers testable methods only, leaving setup and teardown
outside the new timeout. Right?
For example, KafkaSinkITCase.tearDown() waits on topic deletion through
result.all().get().
Could the description explicitly document this scope and the remaining
reliance on CI-level timeout handling for lifecycle hangs?
##########
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:
The ticket/commit/comment say “without a stack trace”. Not arguing that
stack trace is useful, however, I opened failed CI build from July
https://github.com/apache/flink-connector-kafka/actions/runs/29204695279/job/86681985234
and it already has thread dump including stack
```
"main" #1 prio=5 os_prio=0 cpu=9539.69ms elapsed=2413.56s
tid=0x00007f407c029db0 nid=0xa504 waiting on condition [0x00007f4083511000]
java.lang.Thread.State: TIMED_WAITING (sleeping)
at java.lang.Thread.sleep([email protected]/Native Method)
at
org.apache.flink.streaming.api.operators.collect.CollectResultFetcher.sleepBeforeRetry(CollectResultFetcher.java:249)
at
org.apache.flink.streaming.api.operators.collect.CollectResultFetcher.next(CollectResultFetcher.java:115)
at
org.apache.flink.streaming.api.operators.collect.CollectResultIterator.nextResultFromFetcher(CollectResultIterator.java:124)
at
org.apache.flink.streaming.api.operators.collect.CollectResultIterator.hasNext(CollectResultIterator.java:98)
at
org.apache.flink.connector.testframe.utils.CollectIteratorAssert.compareWithExactlyOnceSemantic(CollectIteratorAssert.java:130)
at
org.apache.flink.connector.testframe.utils.CollectIteratorAssert.matchesRecordsFromSource(CollectIteratorAssert.java:85)
at
org.apache.flink.connector.testframe.testsuites.SourceTestSuiteBase.checkResultWithSemantic(SourceTestSuiteBase.java:749)
at
org.apache.flink.connector.testframe.testsuites.SourceTestSuiteBase.testIdleReader(SourceTestSuiteBase.java:526)
at
jdk.internal.reflect.NativeMethodAccessorImpl.invoke0([email protected]/Native
Method)
```
So we already get the dump that the style guide relies on.
If I'm not missing something, the actual benefit here is earlier
method-attributed failure reporting and allowing remaining tests to proceed
where possible.
##########
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:
Should we also add
```
junit.jupiter.execution.timeout.threaddump.enabled = true
```
This captures the test JVM's thread stacks before interruption.
The existing CI dump step still runs on build failure, but the affected test
JVM may already have exited or cleanup changed its state.
Capturing a dump at the timeout would complement those CI diagnostics.
[JUnit 5.13.3
documentation](https://docs.junit.org/5.13.3/user-guide/#writing-tests-declarative-timeouts-debugging-thread-dump)
or am i missing something?
##########
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:
Is excluding the E2E modules intentional?
##########
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:
Nit: could the Javadoc mention that this explicit 15-minute value should
stay aligned with the Maven default? It will save up the next person from
changing one and not the other.
--
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]
