zentol commented on code in PR #19445:
URL: https://github.com/apache/flink/pull/19445#discussion_r849239021
##########
pom.xml:
##########
@@ -1562,7 +1567,6 @@ under the License.
<configuration>
<!-- enables TCP/IP communication
between surefire and forked JVM-->
<forkNode
implementation="org.apache.maven.plugin.surefire.extensions.SurefireForkNodeFactory"/>
-
<forkCount>${flink.forkCount}</forkCount>
Review Comment:
there are a number of modules that override the forkCount to 1 using the
general forkCount option (e.g., various connector modules), which afaik would
now always ignored because the execution-specific option takes precedence.
I don't know whether that's a problem though. I haven't seen an increase in
test instabilities when I used these commits in a WIP branch, but those also
didn't ran on Azure.
In any case we either need to remove these now-ignored forkCount settings,
or adjust the poms to override it in the execution instead (or modify the
properties; I think that works as well).
I would be inclined to just remove them and see what happens.
huh...there should also be some forkReuse settings that are now ignored
after fee8217a65bdbcf2eed8b2c8a31852f84f1022ad. Might wanna get rid of those as
well...
##########
pom.xml:
##########
@@ -95,13 +95,18 @@ under the License.
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<hadoop.version>2.8.5</hadoop.version>
+ <flink.XmxITCase>2048m</flink.XmxITCase>
+ <flink.XmxUnitTest>1024m</flink.XmxUnitTest>
Review Comment:
I would be in favor of just hard-coding these; I don't see a need to make
these configurable; you'd rather want to tweak the number of forks.
##########
pom.xml:
##########
@@ -95,13 +95,18 @@ under the License.
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<hadoop.version>2.8.5</hadoop.version>
+ <flink.XmxITCase>2048m</flink.XmxITCase>
+ <flink.XmxUnitTest>1024m</flink.XmxUnitTest>
<!-- Need to use a user property here because the surefire
forkCount is not exposed as a property. With this we
can set
it on the "mvn" commandline in travis. -->
- <flink.forkCount>1C</flink.forkCount>
+ <!-- Number of forkCounts for ITCase and UnitTest should take
into account allocated memory
+ to the jvm (-Xmx) and the available memory on the
machine running the test -->
+ <flink.forkCountITCase>1C</flink.forkCountITCase>
+ <flink.forkCountUnitTest>2C</flink.forkCountUnitTest>
<!-- Allow overriding the fork behaviour for the expensive
tests in flink-tests
to avoid process kills due to container limits on
TravisCI -->
-
<flink.forkCountTestPackage>${flink.forkCount}</flink.forkCountTestPackage>
+
<flink.forkCountTestPackage>${flink.forkCountITCase}</flink.forkCountTestPackage>
Review Comment:
we should get rid of `forkCountTestPackage`; it has been identical to the
usual forkCount for as long as I can remember.
##########
pom.xml:
##########
@@ -95,13 +95,18 @@ under the License.
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<hadoop.version>2.8.5</hadoop.version>
+ <flink.XmxITCase>2048m</flink.XmxITCase>
+ <flink.XmxUnitTest>1024m</flink.XmxUnitTest>
<!-- Need to use a user property here because the surefire
forkCount is not exposed as a property. With this we
can set
it on the "mvn" commandline in travis. -->
- <flink.forkCount>1C</flink.forkCount>
+ <!-- Number of forkCounts for ITCase and UnitTest should take
into account allocated memory
+ to the jvm (-Xmx) and the available memory on the
machine running the test -->
+ <flink.forkCountITCase>1C</flink.forkCountITCase>
+ <flink.forkCountUnitTest>2C</flink.forkCountUnitTest>
Review Comment:
I'm wondering if we shouldn't default to 2/4 respectively and remove the
setting in the CI scripts. 1C/2C is a somewhat dangerous default; we already
had contributors run into issue with that. (in fact I don't think my machine
could handle that if the forks were running at full power).
Technically not related to this PR though 🤷
##########
flink-runtime/src/test/java/org/apache/flink/runtime/taskexecutor/TaskExecutorRecoveryITCase.java:
##########
@@ -58,7 +58,7 @@
/** Recovery tests for {@link TaskExecutor}. */
@ExtendWith(TestLoggerExtension.class)
-class TaskExecutorRecoveryTest {
+class TaskExecutorRecoveryITCase {
Review Comment:
I would like to double-check again whether this is really necessary.
Creating a TaskExecutor on it's own shouldn't be that expensive, and there
are plenty of other unit tests doing that as well.
--
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]
