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

[Spark] Shade the build for Delta Connect projects #3665

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

longvu-db
Copy link
Contributor

@longvu-db longvu-db commented Sep 10, 2024

Which Delta project/connector is this regarding?

  • Spark
  • Standalone
  • Flink
  • Kernel
  • Other (fill in here)

Description

Shade the build of connectServer, connectClient and connectCommon projects, this is to avoid conflicts on libraries that Delta Connect library users and the Delta Connect library itself may both depend on such as protobuf, grpc,...

This is needed so that we can ship the Delta Connect project once it is finished.

We hope that this also remove the need of include com.google.protobuf:protobuf-java:3.25.1 in the --packages to use Delta Connect server and client.

https://docs.delta.io/latest/delta-spark-connect.html#how-to-start-the-spark-server-with-delta

How was this patch tested?

Run sbt assembly on the project connectServer, connectClient and connectCommon, and use the jar tf command to manually inspect the content of the jar files.

Does this PR introduce any user-facing changes?

No.

@longvu-db longvu-db changed the title [Spark] Shade Delta Connect build [Spark] Shade the build for Delta Connect projects Sep 10, 2024
build.sbt Show resolved Hide resolved
build.sbt Outdated
(assembly / assemblyPackageScala / assembleArtifact) := false,
(assembly / assemblyShadeRules) := Seq(
ShadeRule.rename("com.google.**" -> "org.deltaproject.connect.com.google.@1").inAll,
ShadeRule.rename("io.grpc.**" -> "org.deltaproject.connect.io.grpc.@1").inAll,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By inspecting the jar files of the 3 Delta Connect projects after running publishM2, I believe the shading rule are not propagated to dependent projects, so even if only connectCommon uses io.grpc, we still need to add shading rule io.grpc to connectClient since connectClient depends on connectCommon so connectClient includes connectCommon codes and we need to shade those codes too.

@@ -271,6 +280,23 @@ lazy val connectCommon = (project in file("spark-connect/common"))
PB.gens.java -> (Compile / sourceManaged).value,
PB.gens.plugin("grpc-java") -> (Compile / sourceManaged).value
),
Compile / packageBin := assembly.value,
(assembly / test) := { },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically just following other examples in this Delta's build.sbt file

// It was necessary to change default_scala_version to scala213 in build.sbt

Copy link
Contributor

Choose a reason for hiding this comment

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

This empty block { } indicates we're not testing this correct?
If so please add a // TODO inside the block.

Copy link
Contributor Author

@longvu-db longvu-db Sep 20, 2024

Choose a reason for hiding this comment

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

Hey @scottsand-db, I just have a quick question.

Why did we always do (assembly / test) := { } for other projects in build.sbt, so basically no testing task for assembly? Is it because we don't need to do it (like QA-ing near a Delta Release is good enough)?

60a582b#diff-5634c415cd8c8504fdb973a3ed092300b43c4b8fc1e184f7249eb29a55511f91

@xupefei In case we should consider adding some testing tasks for assembly, I will add a TODO.

build.sbt Outdated Show resolved Hide resolved
Copy link
Contributor

@xupefei xupefei left a comment

Choose a reason for hiding this comment

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

Left one comment otherwise lgtm.

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