[SPARK-45464][CORE] Fix network-yarn distribution build #43289

hasnain-db · 2023-10-09T05:15:41Z

What changes were proposed in this pull request?

Exclude an apache license in the packaged jar to fix the build for network-yarn. Also shade the native libraries so they can be used.

Why are the changes needed?

Making a distribution build using the following command currently fails after #43164:

./dev/make-distribution.sh --tgz  -Phive -Phive-thriftserver -Pyarn

Failures look like

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:3.1.0:run (unpack) on project spark-network-yarn_2.13: An Ant BuildException has occured: Error while expanding /Users/hasnain.lakhani/spark/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar
[ERROR] java.nio.file.FileSystemException: /Users/hasnain.lakhani/spark/common/network-yarn/target/exploded/META-INF/license/LICENSE.aix-netbsd.txt: Not a directory
[ERROR] around Ant part ...<unzip src="/Users/hasnain.lakhani/spark/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar" dest="/Users/hasnain.lakhani/spark/common/network-yarn/target/exploded/" />... @ 5:198 in /Users/hasnain.lakhani/spark/common/network-yarn/target/antrun/build-main.xml
[ERROR] -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.apache.maven.plugins:maven-antrun-plugin:3.1.0:run (unpack) on project spark-network-yarn_2.13: An Ant BuildException has occured: Error while expanding /Users/hasnain.lakhani/spark/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar
java.nio.file.FileSystemException: /Users/hasnain.lakhani/spark/common/network-yarn/target/exploded/META-INF/license/LICENSE.aix-netbsd.txt: Not a directory
around Ant part ...<unzip src="/Users/hasnain.lakhani/spark/common/network-yarn/target/scala-2.13/spark-4.0.0-SNAPSHOT-yarn-shuffle.jar" dest="/Users/hasnain.lakhani/spark/common/network-yarn/target/exploded/" />... @ 5:198 in /Users/hasnain.lakhani/spark/common/network-yarn/target/antrun/build-main.xml

After much trial and error (I added a debugging section for the next person who runs into this) I realized the problem is that inside the jar, the META-inf folder looks like this:

META-INF/LICENSE
META-INF/LICENSE.txt
META-INF/license/
META-INF/license/LICENSE.aix-netbsd.txt
META-INF/license/LICENSE.boringssl.txt
META-INF/license/LICENSE.mvn-wrapper.txt
META-INF/license/LICENSE.tomcat-native.txt

Since the filesystem on macosx is case insensitive, this causes issues. Unzipping this fails, as we unzip the file first, then do not create the directory, and fail to unzip the files after that.

This PR proposes to fix it by removing the LICENSE file, as suggested, by adding it to the exclusion list, so the folder gets copied. The contents are the same as LICENSE.txt (which is also just the apache2 license) so this should be fine. We also shade and copy over the new native libraries similar to the existing ones.

Does this PR introduce any user-facing change?

No

How was this patch tested?

CI

I reran

./dev/make-distribution.sh --tgz  -Phive -Phive-thriftserver -Pyarn

and it succeeded

Was this patch authored or co-authored using generative AI tooling?

No

Appendix: debugging

I was able to debug the contents of the jar using jar tf $jar and print out the files using unzip -q -c $jar $path.

To make iterations go faster I went to /Users/hasnain.lakhani/spark/common/network-yarn/target/antrun and edited the ant file by hand then ran ant -f build-main.xml

hasnain-db · 2023-10-09T05:16:07Z

cc: @wangyum @LuciferYang please let me know if this fix works for you

LuciferYang

Another question is, are we sure that network-yarn needs to shade the netty-tcnative-boringssl-static related jars? If indeed necessary, should we rename libnetty_tcnative_* in a similar way to libnetty_transport*?

also cc @pan3793

spark/common/network-yarn/pom.xml

Lines 178 to 185 in ffc6994

    
           <move file="${project.build.directory}/exploded/META-INF/native/libnetty_transport_native_epoll_x86_64.so" 
        
                 tofile="${project.build.directory}/exploded/META-INF/native/lib${spark.shade.native.packageName}_netty_transport_native_epoll_x86_64.so" /> 
        
           <move file="${project.build.directory}/exploded/META-INF/native/libnetty_transport_native_kqueue_x86_64.jnilib" 
        
                 tofile="${project.build.directory}/exploded/META-INF/native/lib${spark.shade.native.packageName}_netty_transport_native_kqueue_x86_64.jnilib" /> 
        
           <move file="${project.build.directory}/exploded/META-INF/native/libnetty_transport_native_epoll_aarch_64.so" 
        
                 tofile="${project.build.directory}/exploded/META-INF/native/lib${spark.shade.native.packageName}_netty_transport_native_epoll_aarch_64.so" /> 
        
           <move file="${project.build.directory}/exploded/META-INF/native/libnetty_transport_native_kqueue_aarch_64.jnilib" 
        
                 tofile="${project.build.directory}/exploded/META-INF/native/lib${spark.shade.native.packageName}_netty_transport_native_kqueue_aarch_64.jnilib" />

common/network-yarn/pom.xml

hasnain-db · 2023-10-09T05:49:55Z

Another question is, are we sure that network-yarn needs to shade the netty-tcnative-boringssl-static related jar packages? If indeed necessary, should we rename libnetty_tcnative_* in a similar way to libnetty_transport*?

I am not sure if it needs to shade them. I'll admit I'm not the most familiar with this aspect of things, happy to try it without. I will say that even with the shading it does seem to work fine (tests in #42685 pass) so aside from this issue I am not sure about the harm of keeping it shaded for consistency

LuciferYang · 2023-10-09T05:57:54Z

This depends on whether the YarnShuffleService needs the tcnative-boringssl related functions. If so, it indeed needs to be shaded, and libnetty_tcnative_* should likely be renamed similarly; if not, these jars can be directly excluded from the shade plugin.

hasnain-db · 2023-10-09T06:01:14Z

This depends on whether the YarnShuffleService needs the tcnative-boringssl related functions. If so, it indeed needs to be shaded, and libnetty_tcnative_* should likely be renamed similarly; if not, these jars can be directly excluded from the shade.

If it's not provided, the functionality will still work, it'll just be slower (falling back to the JDK implementation). I think for now for simplicity's sake (especially since I don't have a great setup for testing yarn beyond unit tests) I think it makes sense to exclude from the shading, that seems simpler. Will try that out

pan3793 · 2023-10-09T06:23:06Z

@hasnain-db I suggest following @LuciferYang's suggestion to follow the existing rule to rename (the prefix must exactly match the relocated package name) the newly introduced native library, it's actually a standard way of creating artifacts including shaded Netty classes and native libraries.

For details, you could check netty's code on loading native libs.

hasnain-db · 2023-10-09T06:40:00Z

I've updated to rename the libraries + use the exclusion, PTAL

cfmcgrady · 2023-10-09T07:22:56Z

we may consider adding these renaming rules to the SBT build.

$ ./build/sbt -Pyarn network-yarn/assembly
$ jar tf common/network-yarn/target/scala-2.13/spark-network-yarn-4.0.0-SNAPSHOT-hadoop3.3.6.jar | grep libnetty
META-INF/native/libnetty_transport_native_epoll_aarch_64.so
META-INF/native/libnetty_transport_native_epoll_x86_64.so
META-INF/native/libnetty_transport_native_kqueue_aarch_64.jnilib
META-INF/native/libnetty_transport_native_kqueue_x86_64.jnilib

LuciferYang · 2023-10-09T07:34:11Z

libnetty_transport_native_kqueue_x86_64

Yes, this needs to be fixed. However, inconsistencies between sbt assembly and maven shaded already existed before adding boringssl , and there are likely more. The main goal of this pr is to fix make-distribution.sh, so I personally prefer to address the differences between sbt assembly and maven shaded in a series of separate PRs.

common/network-yarn/pom.xml

LuciferYang

+1, LGTM if test pass, but I hope to get approve from @srowen before merging.

LICENSE

LuciferYang · 2023-10-09T15:08:03Z

common/network-yarn/pom.xml

                <exclude>META-INF/*.SF</exclude>
                <exclude>META-INF/*.DSA</exclude>
                <exclude>META-INF/*.RSA</exclude>
+                <exclude>META-INF/LICENSE</exclude>


[WARNING] jackson-annotations-2.15.2.jar, jackson-core-2.15.2.jar, jackson-databind-2.15.2.jar, jackson-module-scala_2.13-2.15.2.jar, log4j-1.2-api-2.20.0.jar, log4j-api-2.20.0.jar, log4j-core-2.20.0.jar, log4j-slf4j2-impl-2.20.0.jar, spark-common-utils_2.13-4.0.0-SNAPSHOT.jar, spark-network-common_2.13-4.0.0-SNAPSHOT.jar, spark-network-shuffle_2.13-4.0.0-SNAPSHOT.jar, spark-network-yarn_2.13-4.0.0-SNAPSHOT.jar define 1 overlapping resource: [WARNING] - META-INF/LICENSE

From the compile log, META-INF/LICENSE might come from multiple jars.

Since this exclude is indiscriminate, in this case, all META-INF/LICENSE would be excluded. If it includes a non-Apache LICENSE, it would also be excluded. Would this be a problem @srowen ?

Of course, even without the exclude, there would still be issues currently because multiple files with the same name would overwrite each other, and it seems difficult to determine which META-INF/LICENSE would be retained in the end.

It looks like we include the license if all of those already in the LICENSE file (or no action is required) so it seems OK here. While it's nice to retain the license buried in the artifact, we should be addressing all licenses in the top level LICENSE and license/ dir, rather than rely on this anyway

Understood, thank you for your explanation @srowen

hasnain-db · 2023-10-10T03:32:05Z

@LuciferYang the docker integration tests are consistently failing with

[info] *** 2 SUITES ABORTED ***
[error] Error during tests:
[error] 	org.apache.spark.sql.jdbc.v2.OracleIntegrationSuite
[error] 	org.apache.spark.sql.jdbc.OracleIntegrationSuite

I don't know if they are flaky but it seems to be unrelated to this PR

LuciferYang · 2023-10-10T05:49:12Z

@LuciferYang the docker integration tests are consistently failing with
[info] *** 2 SUITES ABORTED ***
[error] Error during tests:
[error] 	org.apache.spark.sql.jdbc.v2.OracleIntegrationSuite
[error] 	org.apache.spark.sql.jdbc.OracleIntegrationSuite
I don't know if they are flaky but it seems to be unrelated to this PR

#43123 (comment)

friendly ping @dongjoon-hyun, it seems like this is an unstable case? I noticed that you have also encountered similar failures.

@hasnain-db Can you manually re-trigger Run Docker integration tests?

dongjoon-hyun · 2023-10-10T05:51:14Z

Yes, re-trigger will help. I saw the same situation before after upgrading the recent Oracle image, but didn't check the root cause yet, @LuciferYang .

LuciferYang · 2023-10-10T05:52:27Z

Thank you for your confirmation @dongjoon-hyun ~

hasnain-db · 2023-10-10T13:35:03Z

just retriggered. I've done it 3-4 times already though so I'm not sure this will help. will post back when it's done

hasnain-db · 2023-10-10T15:52:38Z

failed again, retrying once more

hasnain-db · 2023-10-10T16:57:42Z

@LuciferYang it failed again :/ is it possible to merge this with that test failing?

LuciferYang

+1, LGTM

LuciferYang · 2023-10-11T02:21:31Z

Merged into master for Spark 4.0, thanks @hasnain-db @srowen @dongjoon-hyun @wangyum @cfmcgrady

LuciferYang · 2023-10-11T02:22:48Z

@LuciferYang it failed again :/ is it possible to merge this with that test failing?

Sorry for the late reply, I was asleep yesterday. I've checked, the failure should be unrelated to this pr. Thanks ~

hasnain-db · 2023-10-11T03:31:31Z

np - thanks for merging!

working

31b0e9b

github-actions bot added the BUILD label Oct 9, 2023

LuciferYang reviewed Oct 9, 2023

View reviewed changes

common/network-yarn/pom.xml Outdated Show resolved Hide resolved

rework

f47370b

hasnain-db requested a review from LuciferYang October 9, 2023 06:40

wangyum reviewed Oct 9, 2023

View reviewed changes

common/network-yarn/pom.xml Outdated Show resolved Hide resolved

LuciferYang previously approved these changes Oct 9, 2023

View reviewed changes

keep license folder not file

c47bd23

hasnain-db requested review from LuciferYang, srowen and wangyum October 9, 2023 14:42

srowen approved these changes Oct 9, 2023

View reviewed changes

LuciferYang reviewed Oct 9, 2023

View reviewed changes

LuciferYang approved these changes Oct 11, 2023

View reviewed changes

LuciferYang closed this in 128d072 Oct 11, 2023

	<move file="${project.build.directory}/exploded/META-INF/native/libnetty_transport_native_epoll_x86_64.so"
	tofile="${project.build.directory}/exploded/META-INF/native/lib${spark.shade.native.packageName}_netty_transport_native_epoll_x86_64.so" />
	<move file="${project.build.directory}/exploded/META-INF/native/libnetty_transport_native_kqueue_x86_64.jnilib"
	tofile="${project.build.directory}/exploded/META-INF/native/lib${spark.shade.native.packageName}_netty_transport_native_kqueue_x86_64.jnilib" />
	<move file="${project.build.directory}/exploded/META-INF/native/libnetty_transport_native_epoll_aarch_64.so"
	tofile="${project.build.directory}/exploded/META-INF/native/lib${spark.shade.native.packageName}_netty_transport_native_epoll_aarch_64.so" />
	<move file="${project.build.directory}/exploded/META-INF/native/libnetty_transport_native_kqueue_aarch_64.jnilib"
	tofile="${project.build.directory}/exploded/META-INF/native/lib${spark.shade.native.packageName}_netty_transport_native_kqueue_aarch_64.jnilib" />

[SPARK-45464][CORE] Fix network-yarn distribution build #43289

[SPARK-45464][CORE] Fix network-yarn distribution build #43289

Uh oh!

Conversation

hasnain-db commented Oct 9, 2023 • 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?

Was this patch authored or co-authored using generative AI tooling?

Appendix: debugging

Uh oh!

hasnain-db commented Oct 9, 2023

Uh oh!

LuciferYang left a comment • 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!

Uh oh!

hasnain-db commented Oct 9, 2023

Uh oh!

LuciferYang commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasnain-db commented Oct 9, 2023

Uh oh!

pan3793 commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasnain-db commented Oct 9, 2023

Uh oh!

cfmcgrady commented Oct 9, 2023

Uh oh!

LuciferYang commented Oct 9, 2023

Uh oh!

Uh oh!

LuciferYang left a comment • 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!

LuciferYang Oct 9, 2023 • 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!

srowen Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

LuciferYang Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

hasnain-db commented Oct 10, 2023

Uh oh!

LuciferYang commented Oct 10, 2023

Uh oh!

dongjoon-hyun commented Oct 10, 2023

Uh oh!

LuciferYang commented Oct 10, 2023

Uh oh!

hasnain-db commented Oct 10, 2023

Uh oh!

hasnain-db commented Oct 10, 2023

Uh oh!

hasnain-db commented Oct 10, 2023

Uh oh!

LuciferYang left a comment

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Oct 11, 2023

Uh oh!

LuciferYang commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasnain-db commented Oct 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

hasnain-db commented Oct 9, 2023 •

edited

Loading

LuciferYang left a comment •

edited

Loading

LuciferYang commented Oct 9, 2023 •

edited

Loading

pan3793 commented Oct 9, 2023 •

edited

Loading

LuciferYang left a comment •

edited

Loading

LuciferYang Oct 9, 2023 •

edited

Loading

LuciferYang commented Oct 11, 2023 •

edited

Loading