Skip to content

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Nov 14, 2020

What changes were proposed in this pull request?

This PR aims to protect Hadoop 2.x profile compilation in Apache Spark 3.1+.

Why are the changes needed?

Since Apache Spark 3.1+ switch our default profile to Hadoop 3, we had better prevent at least compilation error with Hadoop 2.x profile at the PR review phase. Although this is an additional workload, it will finish quickly because it's compilation only.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass the GitHub Action.

./build/sbt -Pyarn -Pmesos -Pkubernetes -Phive -Phive-thriftserver -Phadoop-cloud -Pkinesis-asl -Djava.version=11 -Pscala-2.13 compile test:compile
hadoop-2:
name: Hadoop 2 build
Copy link
Member Author

Choose a reason for hiding this comment

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

We can upgrade from Hadoop 2.7.4 to Hadoop 2.10.x. So, the job name is only mentioning the major version.

@SparkQA
Copy link

SparkQA commented Nov 14, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 14, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35699/

@mridulm
Copy link
Contributor

mridulm commented Nov 14, 2020

Thanks for making this change @dongjoon-hyun !
This applies only to github actions right ? I assumed that currently the procedure was to wait for either jenkins or github actions to pass before merge.
With this change, a 2.x issue would be caught only by github actions, not jenkins.

Or does jenkins leverage the same ?

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 14, 2020

Ya. That's true. However, GitHub Action is very fast and Hadoop 2 build compilation error is easily detected. I means the result comes in 15 minutes and it's hard to consider it as a flaky job.

For Jenkins, IIUC, it's difficult to apply this in PR Builder. Currently, all PR Builders (PRBuilder, K8s IT PRBuilder) are focusing on the default profile because the runs are too heavy. You may ask Shane for new PRBuilder. He knows better than me about the capacity and new PR Builder for Hadoop 2.

@dongjoon-hyun
Copy link
Member Author

BTW, I have only read-only access privilege to see Jenkins configuration.

@mridulm
Copy link
Contributor

mridulm commented Nov 14, 2020

+CC @shaneknapp
Will it be possible to do the same for jenkins ?
It will ensure consistency and no accidental merges.

@mridulm
Copy link
Contributor

mridulm commented Nov 14, 2020

I means the result comes in 15 minutes and it's hard to consider it as a flaky job.

I am concerned sometimes we might just not see it - given it is listed at the bottom of the list (if jenkins gives green).
If we cant do it in both, atleast reordering so that build failures are called out first would be good. Thoughts @dongjoon-hyun ?

@dongjoon-hyun
Copy link
Member Author

Sure! Given the importance of Hadoop 2, it sounds good to me.

@SparkQA
Copy link

SparkQA commented Nov 14, 2020

Test build #131096 has finished for PR 30378 at commit f439d8a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 14, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 14, 2020

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35701/

@SparkQA
Copy link

SparkQA commented Nov 14, 2020

Test build #131098 has finished for PR 30378 at commit 7840d67.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

There looks a legitimate failure but I like this idea +1.

@dongjoon-hyun
Copy link
Member Author

Yes, @HyukjinKwon . The failure is the motive of this PR. We should prevent that happens again.

Screen Shot 2020-11-15 at 6 32 40 PM

@dongjoon-hyun
Copy link
Member Author

Since #30375 is merged now, I retriggered GitHub Action.

cc @mridulm , @HyukjinKwon , @wangyum

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Looks okay, just one question.

@dongjoon-hyun
Copy link
Member Author

It seems that there exists some delay on GitHub Action. I retriggered GitHub Action because the previous run fails with compilation error again.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 16, 2020

BTW, @HyukjinKwon . Do you still want to use Java 8 in this PR?

Scala 2.13 build job is also using Java 11.

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Nov 16, 2020

Thank you, @wangyum . I'm looking at both this PR and vanilla master branch, too.

The previous failure might happen by another unknown reason instead of the API issue.

@dongjoon-hyun
Copy link
Member Author

It's weird. The new code is not consumed at the GitHub Action re-trigger. I'll rebase this to the master~

[error] /home/runner/work/spark/spark/resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnAllocator.scala:320:52: type mismatch;
706
[error]  found   : Long
707
[error]  required: Int
708
[error]         Resource.newInstance(resourcesWithDefaults.totalMemMiB, resourcesWithDefaults.cores)
709
[error]                                                    ^
710
[error] one error found

@wangyum
Copy link
Member

wangyum commented Nov 16, 2020

Thank you @dongjoon-hyun.

@dongjoon-hyun
Copy link
Member Author

Sorry for the rebasing after your approval, @HyukjinKwon , @wangyum , @viirya . It was inevitable to bring the latest master branch. (I didn't notice that GitHub Action doesn't use the latest master.)

Also, I switched to Java 8 according to @HyukjinKwon 's advice.

@dongjoon-hyun
Copy link
Member Author

Now, the new job is green.
Screen Shot 2020-11-15 at 9 46 58 PM

@dongjoon-hyun
Copy link
Member Author

Could you approve once more please?

@HyukjinKwon
Copy link
Member

Merged to master.

@dongjoon-hyun
Copy link
Member Author

Yay! Thank you, @viirya and @HyukjinKwon ! 😄

@dongjoon-hyun dongjoon-hyun deleted the SPARK-33454 branch November 16, 2020 06:11
@SparkQA
Copy link

SparkQA commented Nov 16, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 16, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/35735/

@SparkQA
Copy link

SparkQA commented Nov 16, 2020

Test build #131132 has finished for PR 30378 at commit f26fc30.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@wangyum
Copy link
Member

wangyum commented Nov 17, 2020

Another related question, do you think we should test Java 14 by Github Action?

@HyukjinKwon
Copy link
Member

There's a discussion thread in the mailing list about JDK 14 FYI: http://apache-spark-developers-list.1001551.n3.nabble.com/Spark-on-JDK-14-td30348.html

@dongjoon-hyun
Copy link
Member Author

Although it's important to track upstream, Java 14 is already EOL.
Only Java 8 / Java 11 / Java 17 (September 2021) are LTS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants