Skip to content
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

[CELEBORN-885][SPARK] Shade RoaringBitmap to avoid dependency conflicts #1803

Closed
wants to merge 5 commits into from

Conversation

FMX
Copy link
Contributor

@FMX FMX commented Aug 10, 2023

What changes were proposed in this pull request?

Shade roaring bitmap to void dependency conflicts.

Why are the changes needed?

Some user reports that celeborn client will introduce roaring bitmap conflicts.

Does this PR introduce any user-facing change?

NO.

How was this patch tested?

GA and cluster.

@pan3793
Copy link
Member

pan3793 commented Aug 10, 2023

sbt should be updated too, cc @cfmcgrady

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #1803 (bc34be2) into main (6ea1ee2) will decrease coverage by 0.00%.
Report is 6 commits behind head on main.
The diff coverage is n/a.

❗ Current head bc34be2 differs from pull request most recent head b3b5973. Consider uploading reports for the commit b3b5973 to get more accurate results

@@            Coverage Diff             @@
##             main    #1803      +/-   ##
==========================================
- Coverage   46.60%   46.59%   -0.00%     
==========================================
  Files         162      162              
  Lines       10079    10078       -1     
  Branches      928      927       -1     
==========================================
- Hits         4696     4695       -1     
+ Misses       5073     5072       -1     
- Partials      310      311       +1     

see 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cfmcgrady
Copy link
Contributor

cfmcgrady commented Aug 10, 2023

Would you mind updating the file project/CelebornBuild.scala, following the same rule as we do in the Flink assembly project?

           val cp = (assembly / fullClasspath).value
           cp filter { v =>
             val name = v.data.getName
-            !(name.startsWith("celeborn-") || name.startsWith("protobuf-java-") ||
-              name.startsWith("guava-") || name.startsWith("netty-") || name.startsWith("commons-lang3-"))
+            !(name.startsWith("celeborn-") ||
+                name.startsWith("protobuf-java-") ||
+                name.startsWith("guava-") ||
+                name.startsWith("netty-") ||
+                name.startsWith("commons-lang3-") ||
+                name.startsWith("RoaringBitmap-"))
           }
         },

           ShadeRule.rename("com.google.protobuf.**" -> "org.apache.celeborn.shaded.com.google.protobuf.@1").inAll,
           ShadeRule.rename("com.google.common.**" -> "org.apache.celeborn.shaded.com.google.common.@1").inAll,
           ShadeRule.rename("io.netty.**" -> "org.apache.celeborn.shaded.io.netty.@1").inAll,
-          ShadeRule.rename("org.apache.commons.**" -> "org.apache.celeborn.shaded.org.apache.commons.@1").inAll
+          ShadeRule.rename("org.apache.commons.**" -> "org.apache.celeborn.shaded.org.apache.commons.@1").inAll,
+          ShadeRule.rename("org.roaringbitmap.**" -> "org.apache.celeborn.shaded.org.roaringbitmap.@1").inAll
         ),

         (assembly / assemblyMergeStrategy) := {

@pan3793
Copy link
Member

pan3793 commented Aug 10, 2023

BTW, NOTICE for the shaded module should be updated too

@pan3793 pan3793 changed the title [CELEBORN-885][SPARK] Shade roaring bitmap to avoid dependency conflicts [CELEBORN-885][SPARK] Shade RoaringBitmap to avoid dependency conflicts Aug 10, 2023
@FMX
Copy link
Contributor Author

FMX commented Aug 11, 2023

Would you mind updating the file project/CelebornBuild.scala, following the same rule as we do in the Flink assembly project?

           val cp = (assembly / fullClasspath).value
           cp filter { v =>
             val name = v.data.getName
-            !(name.startsWith("celeborn-") || name.startsWith("protobuf-java-") ||
-              name.startsWith("guava-") || name.startsWith("netty-") || name.startsWith("commons-lang3-"))
+            !(name.startsWith("celeborn-") ||
+                name.startsWith("protobuf-java-") ||
+                name.startsWith("guava-") ||
+                name.startsWith("netty-") ||
+                name.startsWith("commons-lang3-") ||
+                name.startsWith("RoaringBitmap-"))
           }
         },

           ShadeRule.rename("com.google.protobuf.**" -> "org.apache.celeborn.shaded.com.google.protobuf.@1").inAll,
           ShadeRule.rename("com.google.common.**" -> "org.apache.celeborn.shaded.com.google.common.@1").inAll,
           ShadeRule.rename("io.netty.**" -> "org.apache.celeborn.shaded.io.netty.@1").inAll,
-          ShadeRule.rename("org.apache.commons.**" -> "org.apache.celeborn.shaded.org.apache.commons.@1").inAll
+          ShadeRule.rename("org.apache.commons.**" -> "org.apache.celeborn.shaded.org.apache.commons.@1").inAll,
+          ShadeRule.rename("org.roaringbitmap.**" -> "org.apache.celeborn.shaded.org.roaringbitmap.@1").inAll
         ),

         (assembly / assemblyMergeStrategy) := {

OK,I‘ll update it.

BTW, NOTICE for the shaded module should be updated too

RoaringBitmap has no notice and its using Apache Licencese V2. We already contains its license info in LICENSE-binary file.

Would you mind updating the file project/CelebornBuild.scala, following the same rule as we do in the Flink assembly project?

           val cp = (assembly / fullClasspath).value
           cp filter { v =>
             val name = v.data.getName
-            !(name.startsWith("celeborn-") || name.startsWith("protobuf-java-") ||
-              name.startsWith("guava-") || name.startsWith("netty-") || name.startsWith("commons-lang3-"))
+            !(name.startsWith("celeborn-") ||
+                name.startsWith("protobuf-java-") ||
+                name.startsWith("guava-") ||
+                name.startsWith("netty-") ||
+                name.startsWith("commons-lang3-") ||
+                name.startsWith("RoaringBitmap-"))
           }
         },

           ShadeRule.rename("com.google.protobuf.**" -> "org.apache.celeborn.shaded.com.google.protobuf.@1").inAll,
           ShadeRule.rename("com.google.common.**" -> "org.apache.celeborn.shaded.com.google.common.@1").inAll,
           ShadeRule.rename("io.netty.**" -> "org.apache.celeborn.shaded.io.netty.@1").inAll,
-          ShadeRule.rename("org.apache.commons.**" -> "org.apache.celeborn.shaded.org.apache.commons.@1").inAll
+          ShadeRule.rename("org.apache.commons.**" -> "org.apache.celeborn.shaded.org.apache.commons.@1").inAll,
+          ShadeRule.rename("org.roaringbitmap.**" -> "org.apache.celeborn.shaded.org.roaringbitmap.@1").inAll
         ),

         (assembly / assemblyMergeStrategy) := {

I'll update it.

@FMX
Copy link
Contributor Author

FMX commented Aug 11, 2023

BTW, NOTICE for the shaded module should be updated too

RoarbingBitmap has no notice and its using apache license. We already contains its license info in LICENSE-binary. Is there anything else need to put in NOTICE?

@pan3793
Copy link
Member

pan3793 commented Aug 11, 2023

Not the NOTICE-binary, technically, each jar we published to maven central should contain its own NOTICE file, which reflects the real content itself. For shaded jars, we need to maintain the NOTICE file manually.

@FMX
Copy link
Contributor Author

FMX commented Aug 11, 2023

Not the NOTICE-binary, technically, each jar we published to maven central should contain its own NOTICE file, which reflects the real content itself. For shaded jars, we need to maintain the NOTICE file manually.

Any reference for bundle notice into jar file?

@FMX FMX closed this in 5462281 Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants