TEZ-4695: Consolidate surefire plugin config to remove redundant surefire blocks#467
Conversation
|
Based on discussion #465 (comment), created separate PR for surefire changes. |
| <forkedProcessTimeoutInSeconds>900</forkedProcessTimeoutInSeconds> | ||
| <testFailureIgnore>true</testFailureIgnore> | ||
| <argLine>-Xmx1024m -XX:+HeapDumpOnOutOfMemoryError</argLine> | ||
| <argLine>${test.jvm.args}</argLine> |
There was a problem hiding this comment.
Having 2 argline is a BUG. The test.jvm.args is overwriting the above argLine i.e. Xmx one
Screenshots:
Before:
After:
There was a problem hiding this comment.
@Aggarwal-Raghav : patch itself looks good to me, only a minor concern: now as -Xmx1024m finally applied post-patch, I'm not sure how that behave pre-patch in terms of max heap? the only reason I'm worried about this is that unit tests look to be slower in this run, e.g. tez-dag took 6min, while in other PRs it was 3-4mins consistently
There was a problem hiding this comment.
If the default Xmx value was behaving well so far (locally and CI) then we could keep it the same for now. We could revisit the size if/when we encounter memory issues.
There was a problem hiding this comment.
agreed, if pre-patch there was no Xmx applied, we can simply remove it now, no further investigation needed
abstractdog
left a comment
abstractdog
left a comment
There was a problem hiding this comment.
LGTM, pending tests
this looks a good improvement with reducing duplicated pom snippets
|
💔 -1 overall
This message was automatically generated. |
pom.xml
Outdated
| <testFailureIgnore>true</testFailureIgnore> | ||
| <argLine>-Xmx1024m -XX:+HeapDumpOnOutOfMemoryError</argLine> | ||
| <argLine>${test.jvm.args}</argLine> | ||
| <argLine>-Xmx1024m -XX:+HeapDumpOnOutOfMemoryError ${test.jvm.args}</argLine> |
There was a problem hiding this comment.
The test.jvm.args property was only meant to be used for surefire. Since we only use it here we could get rid of the property and simply inline the content here.
tez-dag/pom.xml
Outdated
| <environmentVariables> | ||
| <LOG_DIRS>${test.log.dir}</LOG_DIRS> | ||
| <environmentVariables combine.children="append"> | ||
| <TEZ_AM_EXTERNAL_ID>test-external-id</TEZ_AM_EXTERNAL_ID> |
There was a problem hiding this comment.
For simplicity, maybe we could also put this env var in the main pom.xml file.
| <forkedProcessTimeoutInSeconds>900</forkedProcessTimeoutInSeconds> | ||
| <testFailureIgnore>true</testFailureIgnore> | ||
| <argLine>-Xmx1024m -XX:+HeapDumpOnOutOfMemoryError</argLine> | ||
| <argLine>${test.jvm.args}</argLine> |
There was a problem hiding this comment.
If the default Xmx value was behaving well so far (locally and CI) then we could keep it the same for now. We could revisit the size if/when we encounter memory issues.
|
Thanks a lot for the review @abstractdog , @zabetak . Will update the PR in some time. |
| -XX:+HeapDumpOnOutOfMemoryError | ||
| --add-opens=java.base/java.lang=ALL-UNNAMED | ||
| --add-opens=java.base/java.util=ALL-UNNAMED | ||
| --add-opens=java.base/java.io=ALL-UNNAMED |
There was a problem hiding this comment.
In 3rd --add-opens java.base/java.io=ALL-UNNAMED equals was missing in <test.jvm.args> added it.
| <artifactId>tez-tests</artifactId> | ||
|
|
||
| <properties> | ||
| <test.log.dir>${project.build.directory}/logs</test.log.dir> |
There was a problem hiding this comment.
removed <test.log.dir> property from child pom's . The ant plugin in child pom is still using it but the appropriate value is propagated from parent pom to child pom.
|
💔 -1 overall
This message was automatically generated. |
|
neat, and makes pom.xml much cleaner, thanks @Aggarwal-Raghav, merging this soon |
4 participants
The surefire changes are to pass the argline defines in parent pom to child pom and the environment variables defined in parent pom, child pom should inherit them. This will reduce explicit surefire plugin block in child module pom