[SPARK-33079][TESTS] Replace the existing Maven job for Scala 2.13 in Github Actions with SBT job #29958
Conversation
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
cc @HyukjinKwon |
.github/workflows/build_and_test.yml
Outdated
| rm -rf ~/.m2/repository/org/apache/spark | ||
| scala-213-sbt: | ||
| name: Scala 2.13 build with SBT |
There was a problem hiding this comment.
Hm, should we set this job in the PR builder? The (almost) same thing is tested at "Scala 2.13 build". I think we'll set a Jenkins regular job to run the tests at SPARK-33044 as well. I would avoid adding too many jobs here since we're not able to have more resources (see also SPARK-32264).
There was a problem hiding this comment.
The (almost) same thing is tested at "Scala 2.13 build"
The main purpose of adding the build with SBT is not just testing with SBT. As I mentioned, some compilation error happens only with SBT so I'd like detect such error before any changes being merged.
I understand we have not enough resource for GitHub Action but if an additional build is added to the Jenkins regular job, it will take much more longer time doesn't it?
Are there any problem to replace the existing "Scala 2.13 build" with the new one?
There was a problem hiding this comment.
I am okay with replacing the existing "Scala 2.13 build". Seems that'll catch the compilation errors more correctly.
it will take much more longer time doesn't it?
It does takes longer to detect the errors. However, there are many other combinations to test and currently we can't test all combinations. So I think the default common-ish combination was picked to use it as a GitHub Actions and Jenkins PR builder. For example, JDK 11 is not being tested in GitHub Actions and Jenkins PR builder.
There was a problem hiding this comment.
For example, JDK 11 is not being tested in GitHub Actions and Jenkins PR builder.
Yeah, I know but whether build is successfully finish is tested in GitHub Actions so I thought build test (not unit test) should be checked in GitHub Actions too for now.
Anyway, If we can replace the existing "Scala 2.13 build" with the new one, I'll do it.
@dongjoon-hyun Do you have any concern about replacing it?
|
Test build #129470 has finished for PR 29958 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #129568 has finished for PR 29958 at commit
|
| "Number of blockIds is not equal to the number of sequence number ranges") | ||
|
|
||
| override def isValid(): Boolean = true | ||
| override def isValid: Boolean = true |
There was a problem hiding this comment.
Is this change related to the current pr?
There was a problem hiding this comment.
Yes. empty parameter list causes compilation error if we build for Scala 2.13 with SBT.
This is what I'd like to detect by CI.
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
I am good with this change.
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #129581 has finished for PR 29958 at commit
|
|
@sarutak It seems that the title and description of this pr need to be modified |
sarutak
changed the title
Done. |
|
I'll merge this after leaving it some more days in case people have different thoughts. |
| ./dev/change-scala-version.sh 2.13 | ||
| ./build/mvn $MAVEN_CLI_OPTS -DskipTests -Pyarn -Pmesos -Pkubernetes -Phive -Phive-thriftserver -Phadoop-cloud -Djava.version=11 -Pscala-2.13 install | ||
| rm -rf ~/.m2/repository/org/apache/spark | ||
| ./build/sbt -Pyarn -Pmesos -Pkubernetes -Phive -Phive-thriftserver -Phadoop-cloud -Pkinesis-asl -Djava.version=11 -Pscala-2.13 compile test:compile |
There was a problem hiding this comment.
So, we are going to remove packaging and installation (Maven) here?
There was a problem hiding this comment.
Can Maven build and SBT build be executed serially in one GitHub Action？
There was a problem hiding this comment.
I think it wouldn't matter too much. If a job is finished quicker the next job can be triggered
There was a problem hiding this comment.
It seems that 2.13 build action runs faster than other actions. Can we consider this ?
There was a problem hiding this comment.
The number of job is limited in github actions instead of each entire workflow run IIRC.
dongjoon-hyun
left a comment
dongjoon-hyun
left a comment
There was a problem hiding this comment.
+1, LGTM for this approach. Thank you, @sarutak and @HyukjinKwon .
I'll leave this PR to you guys~
|
Can we keep Maven and SBT tests for 2.13? Maven is the build of reference anyway |
|
@srowen, maybe i think we'll have to setup the regular Jenkins job for that like we have done for other profiles. I agree that it's best to keep both Maven and SBT build in Scala 2.13 (and actually we should ideally test all combinations like JDK 11 with running the tests) but the problem is limited resource. The resource is currently all shared in all ASF projects although ASF has an Enterprise plan. Maybe we could focus on PR builders to catch the errors with a minimized set. For reference or other stuff, it might be better to set these jobs in regular Jenkins jobs. |
|
Merged to master. |
6 participants
What changes were proposed in this pull request?
SPARK-32926 added a build test to GitHub Action for Scala 2.13 but it's only with Maven.
As SPARK-32873 reported, some compilation error happens only with SBT so I think we need to add another build test to GitHub Action for SBT.
Unfortunately, we don't have abundant resources for GitHub Actions so instead of just adding the new SBT job, let's replace the existing Maven job with the new SBT job for Scala 2.13.
Why are the changes needed?
To ensure build test passes even with SBT for Scala 2.13.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
GitHub Actions' job.