Copilot commented on code in PR #10590:
URL: https://github.com/apache/gravitino/pull/10590#discussion_r3016753049
##########
bundles/iceberg-gcp-bundle/build.gradle.kts:
##########
@@ -28,13 +26,20 @@ dependencies {
implementation(libs.google.auth.credentials)
implementation(libs.google.auth.http)
implementation(libs.iceberg.gcp.bundle)
+ implementation(libs.guava)
}
-tasks.withType(ShadowJar::class.java) {
+tasks.named<com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar>("shadowJar")
{
isZip64 = true
configurations = listOf(project.configurations.runtimeClasspath.get())
archiveClassifier.set("")
+ // Relocate Guava to avoid version conflicts, but exclude already-shaded
classes from iceberg
+ relocate("com.google.common",
"org.apache.gravitino.shaded.com.google.common") {
Review Comment:
The relocation target namespace is very generic
(org.apache.gravitino.shaded.com.google.common). Other Gravitino shaded
artifacts already relocate into org.apache.gravitino.shaded.*, so adding
another Guava copy here can create duplicate classes / jar-hell when multiple
Gravitino bundles are on the classpath. Consider using a
module/provider-specific prefix (consistent with bundles/gcp-bundle using
org.apache.gravitino.gcp.shaded.*) to avoid collisions (e.g.,
org.apache.gravitino.iceberg.gcp.shaded.com.google.common).
```suggestion
relocate("com.google.common",
"org.apache.gravitino.iceberg.gcp.shaded.com.google.common") {
```
##########
bundles/iceberg-gcp-bundle/build.gradle.kts:
##########
@@ -28,13 +26,20 @@ dependencies {
implementation(libs.google.auth.credentials)
implementation(libs.google.auth.http)
implementation(libs.iceberg.gcp.bundle)
+ implementation(libs.guava)
}
-tasks.withType(ShadowJar::class.java) {
+tasks.named<com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar>("shadowJar")
{
Review Comment:
Using a fully-qualified ShadowJar type in the task configuration is
inconsistent with the other bundle build scripts and makes the script harder to
read. Consider importing ShadowJar and using
tasks.named<ShadowJar>("shadowJar") (or tasks.withType<ShadowJar>()) for
consistency.
##########
bundles/iceberg-gcp-bundle/build.gradle.kts:
##########
@@ -28,13 +26,20 @@ dependencies {
implementation(libs.google.auth.credentials)
implementation(libs.google.auth.http)
implementation(libs.iceberg.gcp.bundle)
+ implementation(libs.guava)
}
-tasks.withType(ShadowJar::class.java) {
+tasks.named<com.github.jengelman.gradle.plugins.shadow.tasks.ShadowJar>("shadowJar")
{
isZip64 = true
configurations = listOf(project.configurations.runtimeClasspath.get())
archiveClassifier.set("")
+ // Relocate Guava to avoid version conflicts, but exclude already-shaded
classes from iceberg
+ relocate("com.google.common",
"org.apache.gravitino.shaded.com.google.common") {
+ // Don't transform iceberg's already-shaded Guava
+ exclude("org.apache.iceberg.gcp.shaded.com.google.common.**")
+ }
Review Comment:
Only relocating `com.google.common` does not fully relocate Guava: the Guava
JAR also contains classes in other `com.google.*` packages (e.g.,
`com.google.thirdparty.publicsuffix`). Those would remain in their original
packages in the shaded output and can still conflict with other Guava copies on
the classpath. Consider relocating the additional Guava-owned packages as well
(at least `com.google.thirdparty`) so the bundle doesn’t ship any unrelocated
Guava classes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
