[#1567] fix(spark): Let Spark use its own NettyUtils #1565
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1565 +/- ##
============================================
+ Coverage 53.72% 54.66% +0.94%
- Complexity 2838 2840 +2
============================================
Files 437 417 -20
Lines 24663 22302 -2361
Branches 2094 2094
============================================
- Hits 13250 12192 -1058
+ Misses 10581 9347 -1234
+ Partials 832 763 -69 ☔ View full report in Codecov by Sentry. |
rickyma
force-pushed
the
followup-for-pr-727
branch
2 times, most recently
from
55668e2 to
b7dab8d
Compare
rickyma
force-pushed
the
followup-for-pr-727
branch
8 times, most recently
from
b2aac37 to
28686e6
Compare
rickyma
changed the title
Contributor
Author
rickyma
changed the title
rickyma
force-pushed
the
followup-for-pr-727
branch
2 times, most recently
from
4af531b to
1ced5e7
Compare
rickyma
force-pushed
the
followup-for-pr-727
branch
from
1ced5e7 to
2fa1a11
Compare
jerqi
reviewed
Mar 8, 2024
client-spark/spark2-shaded/pom.xml
Outdated
| to="lib${rss.shade.native.packageName}_netty_transport_native_kqueue_aarch_64.jnilib" | ||
| type="glob"></mapper> | ||
| </move> | ||
| <!-- Delete NettyUtils, let Spark use its own NettyUtils --> |
Contributor
There was a problem hiding this comment.
Add the comment to explain why we do this.
jerqi
approved these changes
Mar 8, 2024
rickyma
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
When we release the shaded client jar for Spark 2.x, the class
org.apache.spark.network.util.NettyUtils.classshould not be included in the package.Why are the changes needed?
Fix #1567.
&
It's also a followup PR for #727.
When running in Spark 2.4, we will encounter exceptions as below:
This is because the return value of
createEventLoopinNettyUtilswithin Uniffle isorg.apache.uniffle.io.netty.channel.EventLoopGroup(which is shaded), while the return value ofcreateEventLoopinNettyUtilswithin Spark isio.netty.channel.EventLoopGroup. When running a Spark application, the Driver loadsNettyUtilsfrom the rss-client's JAR, causing inconsistency in the method's return values and ultimately leading to aNoSuchMethodErrorexception.We should let Spark use its own
NettyUtilsinstead of ours.However, if we simply remove the
org.apache.spark.network.util.NettyUtilsfile from the code repository, we will encounter errors when running integration tests.This is because our code project globally controls the version of Netty in the root
pom.xml'sdependencyManagement, which leads to Spark2's own lower version of Netty being upgraded to a higher version. This causes exceptions due to Netty version incompatibility, resulting in certain fields not being found. This issue does not occur in the production environment, as Spark has its ownNettyUtilsand does not need to use our providedNettyUtils. Retainingorg.apache.spark.network.util.NettyUtilsis somewhat of a workaround for passing integration tests. But given that Spark2 is not frequently updated anymore, maintaining a static version ofNettyUtilsshould not pose a significant problem.Of course, the optimal approach would be to shade our own Netty during integration testing, allowing Spark to continue using its own Netty dependency, effectively separating the two. This would provide the most accurate testing, as any changes in Spark2's Netty version could be verified through unit tests. However, this would mean that a large amount of integration test code would need to prefix
org.apache.uniffleto theimportstatements where Netty is used. Ultimately, this could lead to significant redundancy in the code and cause confusion for those who write codes in the future.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing UTs.