-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-29991][INFRA] Support Hive 1.2 and Hive 2.3 (default) in PR builder #26710
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
Conversation
test-hive1.2 and test-hive2.3 (default) in PR buildertest-hive1.2 and test-hive2.3 (default) in PR builder
|
cc @dongjoon-hyun, @wangyum, and @srowen |
test-hive1.2 and test-hive2.3 (default) in PR buildertest-hive1.2 and test-hive2.3 (default) in PR builder
| "org.apache.spark.sql.hive.thriftserver.ThriftServerQueryTestSuite", | ||
| "org.apache.spark.sql.hive.thriftserver.SparkSQLEnvSuite" | ||
| "org.apache.spark.sql.hive.thriftserver.SparkSQLEnvSuite", | ||
| "org.apache.spark.sql.hive.thriftserver.ui.ThriftServerPageSuite" |
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.
This fixes the test failure against Hive 2.3 (tested in #26706); however, I have no explicit evidence. Just given my speculation and it worked.
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.
I'm still seeing failures like in https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4953/testReport/ - should we make all the thriftserver tests single-threaded?
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.
Ah yeah. Maybe that's a better idea. I'll monitor a bit more and make a PR soon
test-hive1.2 and test-hive2.3 (default) in PR buildertest-hive1.2 and test-hive2.3 (default) in PR builder
test-hive1.2 and test-hive2.3 (default) in PR buildertest-hive1.2 and test-hive2.3 (default) in PR builder
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.
test-hive1.2 and test-hive2.3 (default) in PR builder47e7c42 to
2135955
Compare
This comment has been minimized.
This comment has been minimized.
| if "test-hive1.2" in ghprb_pull_title: | ||
| os.environ["AMPLAB_JENKINS_BUILD_HIVE_PROFILE"] = "hive1.2" | ||
| if "test-hive2.3" in ghprb_pull_title: | ||
| os.environ["AMPLAB_JENKINS_BUILD_HIVE_PROFILE"] = "hive2.3" |
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.
cc @shaneknapp, maybe we can use it this environment variable (?) later when we consider setting the Jenkins jobs for Hive 1.2/2.3 + Hadoop 2.7/3.2 + JDK 8/11 combinations.
The env name looks a bit odd given AMPLAB_JENKINS_BUILD_PROFILE. We might have to rename it later.
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.
sorry, i missed this over the november holiday. yes, i've had a desire for years to rename the AMPLAB_* variables as the amplab project ended in december 2016. :)
i'll do this after the 3.0 cut. no time like the present!
|
Test build #114614 has finished for PR 26710 at commit
|
|
Test build #114618 has finished for PR 26710 at commit
|
srowen
left a comment
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.
Looks reasonable, esp. the last part that may fix the test problem
|
Test build #114621 has finished for PR 26710 at commit
|
|
retest this please |
|
Yeah, seems it fixed. Let me merge this tomorrow if this isn't merged |
dongjoon-hyun
left a comment
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 for the fix!
|
Test build #114635 has finished for PR 26710 at commit
|
|
Do we need to support testing |
|
No because it doesn't work but let's leave it since our build itself already allows such profile combinations. |
|
If you feel strongly, I will make a followup to disallow that specific matrix. Let me know @wangyum. Merged to master. |
|
OK. Thank you @HyukjinKwon LGTM |
|
um, sure... maybe? i'll take a closer look at all of this stuff next week
when the holiday is over (including adding/renaming the jenkins jobs).
…On Fri, Nov 29, 2019 at 12:29 AM Hyukjin Kwon ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In dev/run-tests-jenkins.py
<#26710 (comment)>:
> @@ -182,6 +182,11 @@ def main():
os.environ["AMPLAB_JENKINS_BUILD_PROFILE"] = "hadoop2.7"
if "test-hadoop3.2" in ghprb_pull_title:
os.environ["AMPLAB_JENKINS_BUILD_PROFILE"] = "hadoop3.2"
+ # Switch the Hive profile based on the PR title:
+ if "test-hive1.2" in ghprb_pull_title:
+ os.environ["AMPLAB_JENKINS_BUILD_HIVE_PROFILE"] = "hive1.2"
+ if "test-hive2.3" in ghprb_pull_title:
+ os.environ["AMPLAB_JENKINS_BUILD_HIVE_PROFILE"] = "hive2.3"
cc @shaneknapp <https://github.com/shaneknapp>, maybe we can use it this
environment variable (?) later when we consider setting the Jenkins jobs
for Hive 1.2/2.3 + Hadoop 2.7/3.2 + JDK 8/11 combinations.
The env name looks a bit odd given AMPLAB_JENKINS_BUILD_PROFILE. We might
have to rename it later.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#26710?email_source=notifications&email_token=AAMIHLDTVQOMNYD344ZKMALQWDHHTA5CNFSM4JS35F5KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNMKFAY#pullrequestreview-324575875>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAMIHLAVWZBXFCZ5ADOAVVTQWDHHTANCNFSM4JS35F5A>
.
|
…ilder ### What changes were proposed in this pull request? Currently, Apache Spark PR Builder using `hive-1.2` for `hadoop-2.7` and `hive-2.3` for `hadoop-3.2`. This PR aims to support - `[test-hive1.2]` in PR builder - `[test-hive2.3]` in PR builder to be consistent and independent of the default profile - After this PR, all PR builders will use Hive 2.3 by default (because Spark uses Hive 2.3 by default as of apache@c98e5eb) - Use default profile in AppVeyor build. Note that this was reverted due to unexpected test failure at `ThriftServerPageSuite`, which was investigated in apache#26706 . This PR fixed it by letting it use their own forked JVM. There is no explicit evidence for this fix and it was just my speculation, and thankfully it fixed at least. ### Why are the changes needed? This new tag allows us more flexibility. ### Does this PR introduce any user-facing change? No. (This is a dev-only change.) ### How was this patch tested? Check the Jenkins triggers in this PR. Default: ``` ======================================================================== Building Spark ======================================================================== [info] Building Spark using SBT with these arguments: -Phadoop-2.7 -Phive-2.3 -Phive-thriftserver -Pmesos -Pspark-ganglia-lgpl -Phadoop-cloud -Phive -Pkubernetes -Pkinesis-asl -Pyarn test:package streaming-kinesis-asl-assembly/assembly ``` `[test-hive1.2][test-hadoop3.2]`: ``` ======================================================================== Building Spark ======================================================================== [info] Building Spark using SBT with these arguments: -Phadoop-3.2 -Phive-1.2 -Phadoop-cloud -Pyarn -Pspark-ganglia-lgpl -Phive -Phive-thriftserver -Pmesos -Pkubernetes -Pkinesis-asl test:package streaming-kinesis-asl-assembly/assembly ``` `[test-maven][test-hive-2.3]`: ``` ======================================================================== Building Spark ======================================================================== [info] Building Spark using Maven with these arguments: -Phadoop-2.7 -Phive-2.3 -Pspark-ganglia-lgpl -Pyarn -Phive -Phadoop-cloud -Pkinesis-asl -Pmesos -Pkubernetes -Phive-thriftserver clean package -DskipTests ``` Closes apache#26710 from HyukjinKwon/SPARK-29991. Authored-by: HyukjinKwon <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>
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.
@wangyum @HyukjinKwon could you help update the website https://spark.apache.org/developer-tools.html for including these new tags?
|
+1 for @gatorsmile 's advice. |
|
Sure, will do. |
…ranch-3.0-test-sbt-hadoop-2.7-hive-2.3 ### What changes were proposed in this pull request? This PR tries #26710 (comment) way to fix the test. ### Why are the changes needed? To make the tests pass. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Jenkins will test first, and then `on spark-branch-3.0-test-sbt-hadoop-2.7-hive-2.3` will test it out. Closes #27513 from HyukjinKwon/test-SPARK-30756. Authored-by: HyukjinKwon <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>
…ranch-3.0-test-sbt-hadoop-2.7-hive-2.3 ### What changes were proposed in this pull request? This PR tries #26710 (comment) way to fix the test. ### Why are the changes needed? To make the tests pass. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Jenkins will test first, and then `on spark-branch-3.0-test-sbt-hadoop-2.7-hive-2.3` will test it out. Closes #27513 from HyukjinKwon/test-SPARK-30756. Authored-by: HyukjinKwon <[email protected]> Signed-off-by: HyukjinKwon <[email protected]> (cherry picked from commit 8efe367) Signed-off-by: HyukjinKwon <[email protected]>
…ranch-3.0-test-sbt-hadoop-2.7-hive-2.3 ### What changes were proposed in this pull request? This PR tries apache#26710 (comment) way to fix the test. ### Why are the changes needed? To make the tests pass. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Jenkins will test first, and then `on spark-branch-3.0-test-sbt-hadoop-2.7-hive-2.3` will test it out. Closes apache#27513 from HyukjinKwon/test-SPARK-30756. Authored-by: HyukjinKwon <[email protected]> Signed-off-by: HyukjinKwon <[email protected]> (cherry picked from commit 8efe367) Signed-off-by: HyukjinKwon <[email protected]>
What changes were proposed in this pull request?
Currently, Apache Spark PR Builder is using
hive-1.2forhadoop-2.7andhive-2.3forhadoop-3.2. This PR aims to support[test-hive1.2]in PR builder[test-hive2.3]in PR builder to be consistent and independent of the default profileNote that this was reverted due to unexpected test failure at
ThriftServerPageSuite, which was investigated in #26706 . This PR fixed it by letting it use their own forked JVM. There is no explicit evidence for this fix and it was just my speculation, and thankfully it fixed at least.Why are the changes needed?
This new tag allows us more flexibility.
Does this PR introduce any user-facing change?
No. (This is a dev-only change.)
How was this patch tested?
Check the Jenkins triggers in this PR.
Default:
[test-hive1.2][test-hadoop3.2]:[test-maven][test-hive2.3]: