Skip to content

Conversation

@yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Jul 6, 2020

What changes were proposed in this pull request?

This PR set Hadoop 2.7.4 for hive 1.2 profile, because the default Hadoop 3.2.0 is incompatiable

Why are the changes needed?

The test-hive1.2 tag works wrongly these days

[info] Building Spark using SBT with these arguments:  -Phadoop-3.2 -Phive-1.2 -Phadoop-cloud -Phive -Pkinesis-asl -Pkubernetes -Phive-thriftserver -Pyarn -Pspark-ganglia-lgpl -Pmesos test:package streaming-kinesis-asl-assembly/assembly

It blocks #28912 (comment) and #28963 (comment)

Does this PR introduce any user-facing change?

No

How was this patch tested?

passing Jenkins with test-hive-1.2 tag

@yaooqinn
Copy link
Member Author

yaooqinn commented Jul 6, 2020

cc @dongjoon-hyun @cloud-fan @maropu thanks

pom.xml Outdated
<datanucleus-core.version>3.2.10</datanucleus-core.version>
<hadoop.version>2.7.4</hadoop.version>
<curator.version>2.7.1</curator.version>
<commons-io.version>2.4</commons-io.version>
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean the builds now pass even with -Phadoop-3.2 -Phive-1.2 but setting the Hadoop version to 2.7 implicitly?

Copy link
Member

Choose a reason for hiding this comment

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

Ya. This looks completely wrong.

# 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"
os.environ["AMPLAB_JENKINS_BUILD_PROFILE"] = "hadoop2.7"
Copy link
Member

@HyukjinKwon HyukjinKwon Jul 6, 2020

Choose a reason for hiding this comment

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

Let's don't do this but rather fail with an explicit error message. It will be confusing what Hive 1.2 uses its Hadoop version as.

Copy link
Member

Choose a reason for hiding this comment

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

Ya. +1 for @HyukjinKwon 's advice.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1. thanks for the advice

@HyukjinKwon
Copy link
Member

I think you can give a shot to rather ban the profile combination with an explicit error message, for example, by using https://maven.apache.org/enforcer/enforcer-rules/bannedDependencies.html.

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.

Hi, @yaooqinn .
Sorry, but I'm -1 for this PR. hive-1.2 is not a super profile. It should not override other profile.

@dongjoon-hyun
Copy link
Member

One more thing, @yaooqinn ~
Apache Spark community strongly discourages hive-1.2 profile which is pointing unofficial Apache Hive 1.2 fork. It's because it's simply a violation of global Apache project policy. Apache Spark community should not maintain another Apache project's fork.

@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 6, 2020

@dongjoon-hyun shall we remove the [test-hive1.2] support for PR builders?

@HyukjinKwon
Copy link
Member

Nope, I don't think we should remove it now - it should be best to remove it when we completely remove test-hive1.2. Having [test-hive1.2] is only a dev purpose so shouldn't be a big deal.

@cloud-fan
Copy link
Contributor

If a PR wants to test hive 1.2, it must add both [test-hive1.2] and [test-hadoop2.7]?

@dongjoon-hyun
Copy link
Member

Yes, @cloud-fan ~

@SparkQA
Copy link

SparkQA commented Jul 6, 2020

Test build #125016 has finished for PR 29006 at commit 5fed5a2.

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

@yaooqinn yaooqinn changed the title [SPARK-32058][BUILD][SQL][test-hive1.2][FOLLOWUP] Set hadoop 2.7.4 for hive 1.2 profile [SPARK-32058][BUILD][SQL][test-hive1.2][test-hadoop2.7][FOLLOWUP] Set hadoop 2.7.4 for hive 1.2 profile Jul 6, 2020
@yaooqinn
Copy link
Member Author

yaooqinn commented Jul 6, 2020

retest this please

@SparkQA
Copy link

SparkQA commented Jul 6, 2020

Test build #125044 has started for PR 29006 at commit e1db207.

<rules>
<bannedDependencies>
<excludes>
<exclude>org.apache.hadoop:*:[3.2.0,)</exclude>
Copy link
Member

Choose a reason for hiding this comment

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

Does this work in sbt run? It seems that PR builder is banned by this.

[info] Building Spark using SBT with these arguments:  -Phadoop-2.7 -Phive-1.2 -Pspark-ganglia-lgpl -Pyarn -Pkubernetes -Phadoop-cloud -Phive-thriftserver -Phive -Pkinesis-asl -Pmesos test:package streaming-kinesis-asl-assembly/assembly

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not work for sbt runner, and I don't know how to enable this for it. Any advice?

Copy link
Member

Choose a reason for hiding this comment

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

Let's just drop this PR then. We'll deprecate and remove Hive 1.2 profile in the near future anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

LGTM, I'll close this

@dongjoon-hyun
Copy link
Member

BTW, @yaooqinn . Please use a new JIRA ID because banning a dependency is orthogonal to SPARK-32058.

@yaooqinn
Copy link
Member Author

yaooqinn commented Jul 7, 2020

retest this please

@yaooqinn yaooqinn closed this Jul 7, 2020
@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/125182/
Test FAILed.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants