[SPARK-43988][INFRA] Add a daily maven testing GitHub Action job #41529
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
wait #41487 |
LuciferYang
commented
Jun 9, 2023
.github/workflows/build_and_test.yml
Outdated
| run: | | ||
| # Fix for TTY related issues when launching the Ammonite REPL in tests. | ||
| export TERM=vt100 && script -qfc 'echo exit | amm -s' && rm typescript | ||
| # `set -e` to make the exit status as expected due to use `script -q -e -c` to run the commands |
Contributor
Author
There was a problem hiding this comment.
another way is add a new script, maybe named dev/run-connect-maven-tests as follows:
#!/usr/bin/env bash
set -e
# Go to the Spark project root directory
FWDIR="$(cd "`dirname "$0"`"/..; pwd)"
cd "$FWDIR"
export SPARK_HOME=$FWDIR
echo "$SPARK_HOME"
if [[ -z "$JAVA_VERSION" ]]; then
JAVA_VERSION=8
fi
export MAVEN_OPTS="-Xss64m -Xmx2g -XX:ReservedCodeCacheSize=1g -Dorg.slf4j.simpleLogger.defaultLogLevel=WARN"
export MAVEN_CLI_OPTS="--no-transfer-progress"
# 1. Test with -Phive
# It uses Maven's 'install' intentionally, see https://github.com/apache/spark/pull/26414.
build/mvn $MAVEN_CLI_OPTS -DskipTests -Djava.version=${JAVA_VERSION/-ea} install -Phive
build/mvn $MAVEN_CLI_OPTS -Djava.version=${JAVA_VERSION/-ea} test -pl connector/connect/client/jvm -Phive
# 2. Test without -Phive
build/mvn $MAVEN_CLI_OPTS -DskipTests -Djava.version=${JAVA_VERSION/-ea} install -pl assembly
build/mvn $MAVEN_CLI_OPTS -Djava.version=${JAVA_VERSION/-ea} test -pl connector/connect/client/jvmand run: can change to:
# Fix for TTY related issues when launching the Ammonite REPL in tests.
export TERM=vt100 && script -qfc 'echo exit | amm -s' && rm typescript
export JAVA_VERSION=${{ matrix.java }}
./dev/run-connect-maven-tests
TEST_RETCODE=$?
rm -rf ~/.m2/repository/org/apache/spark
exit $TEST_RETCODE
I tested it and it worked, but due to lack of this script in branch-3.4, we need to add "connect-maven" : "false" to build_branch34.yml when using script
Contributor
Author
|
7c0409b rebase to test |
Contributor
Author
|
@HyukjinKwon @dongjoon-hyun Do you think this is necessary? For the |
LuciferYang
requested review from
HyukjinKwon and
dongjoon-hyun
and removed request for
HyukjinKwon
Contributor
Author
|
friendly ping @HyukjinKwon @dongjoon-hyun |
Contributor
Author
|
also cc @hvanhovell , I would like to add a maven test GA task for the connect modules because I think sbt testing always masks some issues(due to the visibility of the classpath) |
Member
|
Don't feel strongly but I think we should better set a regular job ... with a broader scope .. doing this for every PR is I think a bit of overhead. |
Contributor
Author
A daily job and tests all modules using maven? |
Contributor
Author
|
Set to draft first, will be updated to add a maven daily test later |
LuciferYang
commented
Jun 14, 2023
| \"lint\" : \"true\", | ||
| \"k8s-integration-tests\" : \"true\", | ||
| \"breaking-changes-buf\" : \"true\", | ||
| \"maven-build\" : \"true\", |
Contributor
Author
There was a problem hiding this comment.
will change to false after test
| - ${{ inputs.hadoop }} | ||
| hive: | ||
| - hive2.3 | ||
| modules: |
Member
There was a problem hiding this comment.
Do you use modules in this PR?
.github/workflows/build_and_test.yml
Outdated
| export JAVA_VERSION=${{ matrix.java }} | ||
| ./build/mvn $MAVEN_CLI_OPTS -DskipTests -Pyarn -Pmesos -Pkubernetes -Pvolcano -Phive -Phive-thriftserver -Phadoop-cloud -Djava.version=${JAVA_VERSION/-ea} clean install | ||
| if [[ "$INCLUDED_TAGS" != "" ]]; then | ||
| ./build/mvn $MAVEN_CLI_OPTS -pl "$MODULES_TO_TEST" -Pyarn -Pmesos -Pkubernetes -Pvolcano -Phive -Phive-thriftserver -Phadoop-cloud -Djava.version=${JAVA_VERSION/-ea} -Dtest.include.tags="$INCLUDED_TAGS" test |
Member
There was a problem hiding this comment.
Hmm. Got it. It uses MODULES_TO_TEST. May I ask which this is "for connect client module"?
Member
There was a problem hiding this comment.
resource-managers/yarn,resource-managers/mesos,resource-managers/kubernetes is irrelevent to this, isn't it?
Contributor
Author
There was a problem hiding this comment.
Yes, the topic of this pr will be changed and I want to add a daily maven test will to cover all modules.
Due to ongoing testing, the pr title and description have not been changed, which has caused you misunderstandings. Sorry for any inconvenience caused @dongjoon-hyun
LuciferYang
changed the title
Contributor
Author
|
created SPARK-44064 to tracking this |
Contributor
Author
|
created SPARK-44069 to tracking this |
This reverts commit d53ddbe.
LuciferYang
commented
Jun 15, 2023
...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
Show resolved
Hide resolved
…uava Cache"" This reverts commit 1fe8049.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
TBD
Why are the changes needed?
TBD
Does this PR introduce any user-facing change?
No, just for test.
How was this patch tested?