-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-33464][INFRA] Add/remove (un)necessary cache and restructure GitHub Actions yaml #30391
Conversation
- name: Build with SBT | ||
run: | | ||
./dev/change-scala-version.sh 2.13 | ||
./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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dongjoon-hyun, I just move it back below. Seems like it affects when things are ported back (Hadoop 2 build is only in master branch). I think it will still obviously show the red X one of builds fails anyway .. so I think it might be fine to move below here together with Java 11 and Scala 2.13 builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HyukjinKwon . This was added at the end but was moved to the first position due @mridulm 's direct request. (#30378 (comment)).
I'm okay with all positions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mridulm, would you mind moving it below here? It will show an X obviously if one of builds fails anyway. It seems like it can cause some conflicts easily when we backport something to other branches for GitHub Actions. Currently, we're running the builds in all active branches, and porting back things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HyukjinKwon Currently, we merge if either of the builds pass (jenkins or github).
The reason I was requesting @dongjoon-hyun to move it to top was if jenkins gives positive build (since it is not testing hadoop 2), then only way to identify hadoop 2.x failure is by github actions failing on hadoop 2 build.
If we can get jenkins to also build hadoop-2, then we dont need to move this to top.
Else, it is common for reviewers to not see the last entry in github build (when there is a jenkins green).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HyukjinKwon Is there a better solution currently ? I am not an expert on github or jenkins :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mridulm, oh but if there's any of them fails, it shows a clear X like
So it should be fine. I do believe reviewers check the X or test failures from Jenkins before merging it in :-). Actually, Java 11 and Scala 2.13 too. Jenkins PR builder does not run them as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good to me. Thanks for the details @HyukjinKwon !
@dongjoon-hyun given details @HyukjinKwon provided, I am fine with the changes he proposed. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the agreement, shall we merge this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please go ahead @dongjoon-hyun , thanks !
cc @sarutak and @dongjoon-hyun FYI. |
BTW, I will make a backporting PR separately. Most of changes here can be ported back I believe. |
uses: actions/setup-java@v1 | ||
with: | ||
java-version: 11 | ||
java-version: 8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem.
Thank you for this optimization, @HyukjinKwon . 😄 |
Kubernetes integration test starting |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9b65e62
to
8f6d64f
Compare
Please check #30391 (comment) . |
Addressed! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HyukjinKwon . Now, do we need to check only this one from @mridulm ?
Yes, thanks for clarifying it. |
The position is up to the agreement between @HyukjinKwon and @mridulm . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you all!
Merged to master.
…itHub Actions yaml This PR proposes: - Add `~/.sbt` directory into the build cache, see also sbt/sbt#3681 - Move `hadoop-2` below to put up together with `java-11` and `scala-213`, see apache#30391 (comment) - Remove unnecessary `.m2` cache if you run SBT tests only. - Remove `rm ~/.m2/repository/org/apache/spark`. If you don't `sbt publishLocal` or `mvn install`, we don't need to care about it. - Use Java 8 in Scala 2.13 build. We can switch the Java version to 11 used for release later. - Add caches into linters. The linter scripts uses `sbt` in, for example, `./dev/lint-scala`, and uses `mvn` in, for example, `./dev/lint-java`. Also, it requires to `sbt package` in Jekyll build, see: https://github.com/apache/spark/blob/master/docs/_plugins/copy_api_dirs.rb#L160-L161. We need full caches here for SBT, Maven and build tools. - Use the same syntax of Java version, 1.8 -> 8. - Remove unnecessary stuff - Cache what we can in the build No, dev-only. It will be tested in GitHub Actions build at the current PR Closes apache#30391 from HyukjinKwon/SPARK-33464. Authored-by: HyukjinKwon <gurwls223@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…itHub Actions yaml This PR proposes: - Add `~/.sbt` directory into the build cache, see also sbt/sbt#3681 - Move `hadoop-2` below to put up together with `java-11` and `scala-213`, see apache#30391 (comment) - Remove unnecessary `.m2` cache if you run SBT tests only. - Remove `rm ~/.m2/repository/org/apache/spark`. If you don't `sbt publishLocal` or `mvn install`, we don't need to care about it. - Use Java 8 in Scala 2.13 build. We can switch the Java version to 11 used for release later. - Add caches into linters. The linter scripts uses `sbt` in, for example, `./dev/lint-scala`, and uses `mvn` in, for example, `./dev/lint-java`. Also, it requires to `sbt package` in Jekyll build, see: https://github.com/apache/spark/blob/master/docs/_plugins/copy_api_dirs.rb#L160-L161. We need full caches here for SBT, Maven and build tools. - Use the same syntax of Java version, 1.8 -> 8. - Remove unnecessary stuff - Cache what we can in the build No, dev-only. It will be tested in GitHub Actions build at the current PR Closes apache#30391 from HyukjinKwon/SPARK-33464. Authored-by: HyukjinKwon <gurwls223@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…ure GitHub Actions yaml ### What changes were proposed in this pull request? This PR backports #30391. Note that it's a partial backport. This PR proposes: - Add `~/.sbt` directory into the build cache, see also sbt/sbt#3681 - ~Move `hadoop-2` below to put up together with `java-11` and `scala-213`, see #30391 (comment) - Remove unnecessary `.m2` cache if you run SBT tests only. - Remove `rm ~/.m2/repository/org/apache/spark`. If you don't `sbt publishLocal` or `mvn install`, we don't need to care about it. - ~Use Java 8 in Scala 2.13 build. We can switch the Java version to 11 used for release later.~ - Add caches into linters. The linter scripts uses `sbt` in, for example, `./dev/lint-scala`, and uses `mvn` in, for example, `./dev/lint-java`. Also, it requires to `sbt package` in Jekyll build, see: https://github.com/apache/spark/blob/master/docs/_plugins/copy_api_dirs.rb#L160-L161. We need full caches here for SBT, Maven and build tools. - Use the same syntax of Java version, 1.8 -> 8. ### Why are the changes needed? - Remove unnecessary stuff - Cache what we can in the build ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? It will be tested in GitHub Actions build at the current PR Closes #30416 from HyukjinKwon/SPARK-33464-3.0. Authored-by: HyukjinKwon <gurwls223@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
…ure GitHub Actions yaml ### What changes were proposed in this pull request? This PR backports #30391. Note that it's a partial backport. This PR proposes: - Add `~/.sbt` directory into the build cache, see also sbt/sbt#3681 - ~Move `hadoop-2` below to put up together with `java-11` and `scala-213`, see #30391 (comment) - Remove unnecessary `.m2` cache if you run SBT tests only. - Remove `rm ~/.m2/repository/org/apache/spark`. If you don't `sbt publishLocal` or `mvn install`, we don't need to care about it. - ~Use Java 8 in Scala 2.13 build. We can switch the Java version to 11 used for release later.~ - Add caches into linters. The linter scripts uses `sbt` in, for example, `./dev/lint-scala`, and uses `mvn` in, for example, `./dev/lint-java`. Also, it requires to `sbt package` in Jekyll build, see: https://github.com/apache/spark/blob/master/docs/_plugins/copy_api_dirs.rb#L160-L161. We need full caches here for SBT, Maven and build tools. - Use the same syntax of Java version, 1.8 -> 8. ### Why are the changes needed? - Remove unnecessary stuff - Cache what we can in the build ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? It will be tested in GitHub Actions build at the current PR Closes #30417 from HyukjinKwon/SPARK-33464-2.4. Authored-by: HyukjinKwon <gurwls223@apache.org> Signed-off-by: HyukjinKwon <gurwls223@apache.org>
What changes were proposed in this pull request?
This PR proposes:
~/.sbt
directory into the build cache, see also Directory standard sbt/sbt#3681hadoop-2
below to put up together withjava-11
andscala-213
, see [SPARK-33464][INFRA] Add/remove (un)necessary cache and restructure GitHub Actions yaml #30391 (comment).m2
cache if you run SBT tests only.rm ~/.m2/repository/org/apache/spark
. If you don'tsbt publishLocal
ormvn install
, we don't need to care about it.sbt
in, for example,./dev/lint-scala
, and usesmvn
in, for example,./dev/lint-java
. Also, it requires tosbt package
in Jekyll build, see: https://github.com/apache/spark/blob/master/docs/_plugins/copy_api_dirs.rb#L160-L161. We need full caches here for SBT, Maven and build tools.Why are the changes needed?
Does this PR introduce any user-facing change?
No, dev-only.
How was this patch tested?
It will be tested in GitHub Actions build at the current PR