[SPARK-33212][FOLLOW-UP][BUILD] Bring back duplicate dependency check and add more strict Hadoop version check #31203

sunchao · 2021-01-15T23:53:39Z

What changes were proposed in this pull request?

Add back Maven enforcer for duplicate dependencies check
More strict check on Hadoop versions which support shaded client in IsolatedClientLoader. To do proper version check, this adds a util function majorMinorPatchVersion to extract major/minor/patch version from a string.
Cleanup unnecessary code

Why are the changes needed?

The Maven enforcer was removed as part of #30556. This proposes to add it back.

Also, Hadoop shaded client doesn't work in certain cases (see these comments for details). This strictly checks that the current Hadoop version (i.e., 3.2.2 at the moment) has good support of shaded client or otherwise fallback to old unshaded ones.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

xkrogen

Thanks for the fast follow-on Chao!

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

SparkQA · 2021-01-16T02:20:58Z

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

SparkQA · 2021-01-16T02:46:12Z

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

SparkQA · 2021-01-16T03:35:41Z

Test build #134130 has finished for PR 31203 at commit 062b7bc.

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

SparkQA · 2021-01-16T19:40:09Z

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

SparkQA · 2021-01-16T20:34:05Z

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

SparkQA · 2021-01-17T02:42:24Z

Test build #134146 has finished for PR 31203 at commit b5d82b7.

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

sunchao · 2021-01-22T20:16:26Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

This is probably out of the scope of this PR. I'll open a new one if we agree this is the right thing to do.

Seems a good improvement to me, that table is pretty unsightly as-is.

xkrogen

I wonder if this should go into VersionUtils? That does claim to be specifically for working with Spark version strings, but it seems relevant...

xkrogen · 2021-01-22T20:51:00Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

+  def supportHadoopShadedClient(hadoopVersion: String): Boolean = {
+    getVersionParts(hadoopVersion).exists {
+      case (3, 2, v) if v >= 2 => true
+      case _ => false


maybe

case (maj, _, _) if maj > 3 => true case (3, min, _) if min > 2 => true case (3, 2, patch) if patch >=2 => true

Seems like we can reasonably assume that future versions of Hadoop will support the shaded client?

I think we'd better to wait until the future versions come out before changing this (so that we can verify firs). For instance, Hadoop 3.3.0 currently doesn't support shaded client (due to the hadoop-aws issue). But yeah the Hadoop 3.2.2+ should support the shaded client assuming there's no regression.

Interesting... I just worry about forgetting to update this if/when we bump the Hadoop version in the future and causing a regression. Has the hadoop-aws fix made it to be targeted for Hadoop 3.3.1? If so, can we reasonably assume that 3.2.2+, 3.3.1+, and 3.4.0+ will have it?

It seems you're more tied into what's happening in the Hadoop world than I am these days so I'll take your word in either direction. If we decide not to future-proof it, can we create a follow-up JIRA to revisit it once some future release is out at which time we would be confident in putting a wildcard?

I think your concern is valid. One thing we can do is perhaps adding a test to make sure that the built-in Hadoop version is always compatible with the shaded client. So that in future if we upgrade Hadoop version & forget to do this, the test will break.

And yes we can assume that 3.2.2+, 3.3.1+ and 3.4.0+ will all have the fix.

Excellent idea on adding a compatibility test for the built-in Hadoop version!

xkrogen · 2021-01-22T20:51:30Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

Seems a good improvement to me, that table is pretty unsightly as-is.

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

sunchao · 2021-01-22T21:08:44Z

I wonder if this should go into VersionUtils? That does claim to be specifically for working with Spark version strings, but it seems relevant...

Oops I wasn't even aware of VersionUtils. Yes I agree it seems a better place to put this code. Let me update the PR later.

SparkQA · 2021-01-22T22:58:46Z

Test build #134382 has finished for PR 31203 at commit 6657c87.

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

- move version parsing to `VersionUtils` - add compatibility test - use `Option` for null

SparkQA · 2021-01-24T00:01:07Z

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

SparkQA · 2021-01-24T00:29:25Z

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

SparkQA · 2021-01-24T01:49:13Z

Test build #134405 has finished for PR 31203 at commit 9b8a911.

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

core/src/test/scala/org/apache/spark/util/VersionUtilsSuite.scala

sql/hive/src/test/scala/org/apache/spark/sql/hive/client/HadoopVersionInfoSuite.scala

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

xkrogen · 2021-01-25T16:40:56Z

core/src/main/scala/org/apache/spark/util/VersionUtils.scala

+
+  /**
+   * Retrieves the major, minor and patch parts from the input `version`. Returns `None` if the
+   * input is not of a valid format.


I think we should mention that minor/patch versions are filled in as 0 if they're not found. This is different from the behavior of other methods in this class (e.g. majorMinor will give an error if minor is not present)

Yeah good point.

core/src/main/scala/org/apache/spark/util/VersionUtils.scala

SparkQA · 2021-01-26T12:31:42Z

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

SparkQA · 2021-01-26T14:01:53Z

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

SparkQA · 2021-01-26T14:12:40Z

Test build #134499 has finished for PR 31203 at commit 396130b.

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

xkrogen · 2021-01-26T17:11:08Z

Changes LGTM thanks @sunchao !

sunchao · 2021-01-26T18:14:39Z

Thanks @xkrogen for the thorough review! @dongjoon-hyun @viirya could you take a look?

core/src/main/scala/org/apache/spark/util/VersionUtils.scala

dongjoon-hyun · 2021-01-26T21:16:39Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

+  def supportsHadoopShadedClient(hadoopVersion: String): Boolean = {
+    VersionUtils.majorMinorPatchVersion(hadoopVersion).exists {
+      case (3, 2, v) if v >= 2 => true
+      case _ => false


Got it. I agree that we need to change this again.

And yes we can assume that 3.2.2+, 3.3.1+ and 3.4.0+ will all have the fix.

dongjoon-hyun

+1, LGTM (with only one minor comment about comment typo)

viirya · 2021-01-26T21:33:44Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala

-    val extraExclusions = if (hadoopVersion.startsWith("3")) {
-      // this introduced from lower version of Hive could conflict with jars in Hadoop 3.2+, so
-      // exclude here in favor of the ones in Hadoop 3.2+
-      Seq("org.apache.hadoop:hadoop-auth")
-    } else {


No need to exclude hadoop-auth anymore?

Yea it's no longer needed. Please see here and here for the context.

viirya

looks good with one question.

dongjoon-hyun · 2021-01-26T23:34:39Z

Since the last question is addressed, I'll merge this. Thanks!

…uld support shaded client" for hadoop-2.7 ### What changes were proposed in this pull request? We added test "built-in Hadoop version should support shaded client" in #31203, but it fails when profile hadoop-2.7 is activated. This change fixes the test by skipping the assertion when Hadoop version is 2. ### Why are the changes needed? The test fails in master branch when profile hadoop-2.7 is activated. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Ran the test with hadoop-2.7 profile. Closes #31391 from bozhang2820/fix-hadoop-2-version-test. Authored-by: Bo Zhang <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

… and add more strict Hadoop version check ### What changes were proposed in this pull request? 1. Add back Maven enforcer for duplicate dependencies check 2. More strict check on Hadoop versions which support shaded client in `IsolatedClientLoader`. To do proper version check, this adds a util function `majorMinorPatchVersion` to extract major/minor/patch version from a string. 3. Cleanup unnecessary code ### Why are the changes needed? The Maven enforcer was removed as part of apache#30556. This proposes to add it back. Also, Hadoop shaded client doesn't work in certain cases (see [these comments](apache#30701 (comment)) for details). This strictly checks that the current Hadoop version (i.e., 3.2.2 at the moment) has good support of shaded client or otherwise fallback to old unshaded ones. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing tests. Closes apache#31203 from sunchao/SPARK-33212-followup. Lead-authored-by: Chao Sun <[email protected]> Co-authored-by: Chao Sun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…uld support shaded client" for hadoop-2.7 ### What changes were proposed in this pull request? We added test "built-in Hadoop version should support shaded client" in apache#31203, but it fails when profile hadoop-2.7 is activated. This change fixes the test by skipping the assertion when Hadoop version is 2. ### Why are the changes needed? The test fails in master branch when profile hadoop-2.7 is activated. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Ran the test with hadoop-2.7 profile. Closes apache#31391 from bozhang2820/fix-hadoop-2-version-test. Authored-by: Bo Zhang <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

… and add more strict Hadoop version check 1. Add back Maven enforcer for duplicate dependencies check 2. More strict check on Hadoop versions which support shaded client in `IsolatedClientLoader`. To do proper version check, this adds a util function `majorMinorPatchVersion` to extract major/minor/patch version from a string. 3. Cleanup unnecessary code The Maven enforcer was removed as part of apache#30556. This proposes to add it back. Also, Hadoop shaded client doesn't work in certain cases (see [these comments](apache#30701 (comment)) for details). This strictly checks that the current Hadoop version (i.e., 3.2.2 at the moment) has good support of shaded client or otherwise fallback to old unshaded ones. No. Existing tests. Closes apache#31203 from sunchao/SPARK-33212-followup. Lead-authored-by: Chao Sun <[email protected]> Co-authored-by: Chao Sun <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…uld support shaded client" for hadoop-2.7 ### What changes were proposed in this pull request? We added test "built-in Hadoop version should support shaded client" in apache#31203, but it fails when profile hadoop-2.7 is activated. This change fixes the test by skipping the assertion when Hadoop version is 2. ### Why are the changes needed? The test fails in master branch when profile hadoop-2.7 is activated. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Ran the test with hadoop-2.7 profile. Closes apache#31391 from bozhang2820/fix-hadoop-2-version-test. Authored-by: Bo Zhang <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

github-actions bot added BUILD SQL labels Jan 15, 2021

xkrogen suggested changes Jan 16, 2021

View reviewed changes

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/IsolatedClientLoader.scala Outdated Show resolved Hide resolved

sunchao changed the title ~~[SPARK-33212][FOLLOW-UP][BUILD] Bring back duplicate dependency check and add more strict Hadoop version check~~ [SPARK-33212][FOLLOW-UP][test-maven][test-hadoop2.7] Bring back duplicate dependency check and add more strict Hadoop version check Jan 16, 2021

sunchao changed the title ~~[SPARK-33212][FOLLOW-UP][test-maven][test-hadoop2.7] Bring back duplicate dependency check and add more strict Hadoop version check~~ [SPARK-33212][FOLLOW-UP][BUILD] Bring back duplicate dependency check and add more strict Hadoop version check Jan 22, 2021

sunchao force-pushed the SPARK-33212-followup branch from b5d82b7 to 8650a73 Compare January 22, 2021 20:15

sunchao commented Jan 22, 2021

View reviewed changes

sunchao and others added 2 commits January 22, 2021 20:17

SPARK-33212 follow-up

b62b58d

add version parsing

6657c87

sunchao force-pushed the SPARK-33212-followup branch from 8650a73 to 6657c87 Compare January 22, 2021 20:18

xkrogen suggested changes Jan 22, 2021

View reviewed changes

address comments

9b8a911

- move version parsing to `VersionUtils` - add compatibility test - use `Option` for null

github-actions bot added the CORE label Jan 23, 2021

xkrogen suggested changes Jan 25, 2021

View reviewed changes

address comments & remove hive changes

396130b

sunchao marked this pull request as ready for review January 26, 2021 10:43

dongjoon-hyun reviewed Jan 26, 2021

View reviewed changes

core/src/main/scala/org/apache/spark/util/VersionUtils.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jan 26, 2021

View reviewed changes

dongjoon-hyun approved these changes Jan 26, 2021

View reviewed changes

fix comment

43367c5

viirya reviewed Jan 26, 2021

View reviewed changes

viirya approved these changes Jan 26, 2021

View reviewed changes

dongjoon-hyun closed this in abf7e81 Jan 26, 2021

bozhang2820 mentioned this pull request Jan 29, 2021

[SPARK-33212][FOLLOW-UP][BUILD] Fix test "built-in Hadoop version should support shaded client" for hadoop-2.7 #31391

Closed

[SPARK-33212][FOLLOW-UP][BUILD] Bring back duplicate dependency check and add more strict Hadoop version check #31203

[SPARK-33212][FOLLOW-UP][BUILD] Bring back duplicate dependency check and add more strict Hadoop version check #31203

Uh oh!

Conversation

sunchao commented Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

xkrogen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Jan 16, 2021

Uh oh!

SparkQA commented Jan 16, 2021

Uh oh!

SparkQA commented Jan 16, 2021

Uh oh!

SparkQA commented Jan 16, 2021

Uh oh!

SparkQA commented Jan 16, 2021

Uh oh!

SparkQA commented Jan 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xkrogen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xkrogen Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sunchao commented Jan 22, 2021

Uh oh!

SparkQA commented Jan 22, 2021

Uh oh!

SparkQA commented Jan 24, 2021

Uh oh!

SparkQA commented Jan 24, 2021

Uh oh!

SparkQA commented Jan 24, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Jan 26, 2021

Uh oh!

SparkQA commented Jan 26, 2021

Uh oh!

SparkQA commented Jan 26, 2021

Uh oh!

xkrogen commented Jan 26, 2021

Uh oh!

sunchao commented Jan 26, 2021

Uh oh!

Uh oh!

sunchao commented Jan 15, 2021 •

edited

Loading

xkrogen Jan 22, 2021 •

edited

Loading