[GitHub] [flink] zentol commented on a diff in pull request #19445: [FLINK-27209][build] Half the memory and double forkCount for running unit tests

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org