Skip to content
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

Closed
wants to merge 3 commits into from

Conversation

HyukjinKwon
Copy link
Member

@HyukjinKwon HyukjinKwon commented Nov 17, 2020

What changes were proposed in this pull request?

This PR proposes:

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

@github-actions github-actions bot added the INFRA label Nov 17, 2020
- 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

Copy link
Member Author

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.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Nov 17, 2020

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.

Copy link
Member Author

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.

Copy link
Contributor

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).

Copy link
Contributor

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 :-)

Copy link
Member Author

@HyukjinKwon HyukjinKwon Nov 18, 2020

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

Screen Shot 2020-11-18 at 9 07 02 PM

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.

Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Member

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?

Copy link
Contributor

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 !

@HyukjinKwon
Copy link
Member Author

cc @sarutak and @dongjoon-hyun FYI.

@HyukjinKwon
Copy link
Member Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem.

@dongjoon-hyun
Copy link
Member

Thank you for this optimization, @HyukjinKwon . 😄

@SparkQA
Copy link

SparkQA commented Nov 17, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35797/

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@dongjoon-hyun
Copy link
Member

Please check #30391 (comment) .

@HyukjinKwon
Copy link
Member Author

Addressed!

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

@SparkQA

This comment has been minimized.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HyukjinKwon
Copy link
Member Author

Yes, thanks for clarifying it.

@dongjoon-hyun
Copy link
Member

The position is up to the agreement between @HyukjinKwon and @mridulm .

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a 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.

HyukjinKwon added a commit to HyukjinKwon/spark that referenced this pull request Nov 19, 2020
…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>
HyukjinKwon added a commit to HyukjinKwon/spark that referenced this pull request Nov 19, 2020
…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>
dongjoon-hyun pushed a commit that referenced this pull request Nov 19, 2020
…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>
HyukjinKwon added a commit that referenced this pull request Nov 19, 2020
…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>
@HyukjinKwon HyukjinKwon deleted the SPARK-33464 branch December 7, 2020 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants