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

Fix potential jetcd-core shading problem #4526

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

Shawyeok
Copy link
Contributor

Motivation

There is a potential jar shading issue introduced in #4426 that causes NoClassDefFoundError when connecting to an etcd metadata store.

The jetcd-core-shaded module was introduced in #4426 to address the compatibility issues between jetcd-core’s grpc-java dependency and Netty. You can find more details here and in the grpc-java documentation.

Currently, we use unpack-shaded-jar execution unpacks the shaded jar produced by maven-shade-plugin:shade into the jetcd-core-shaded/target/classes directory. However, the classes in this directory conflict with its dependencies. If the maven-shade-plugin:shade runs again without cleaning this directory, it can produce an incorrect shaded jar. You can replicate and verify this issue with the following commands:

# Step 1: Clean the build directory
mvn clean

# Step 2: Perform an install and unpack the shaded jar into a directory.
# Verify the import statement for `io.netty.handler.logging.ByteBufFormat` in 
# `org/apache/pulsar/jetcd/shaded/io/vertx/core/net/NetClientOptions.class`. 
# The correct import should be: 
# `import io.grpc.netty.shaded.io.netty.handler.logging.ByteBufFormat;`.
mvn install
unzip $M2_REPO/org/apache/bookkeeper/metadata/drivers/jetcd-core-shaded/4.18.0-SNAPSHOT/jetcd-core-shaded-4.18.0-SNAPSHOT-shaded.jar \
  -d metadata-drivers/jetcd-core-shaded/target/first-classes

# Step 3: Run the install command again without cleaning.
# The unpacked jar from the previous step will persist in `target/classes`. 
# Unpack the shaded jar into a different directory (e.g., second-classes) and check the import.
# The incorrect import will be: 
# `import io.grpc.netty.shaded.io.grpc.netty.shaded.io.netty.handler.logging.ByteBufFormat;`.
mvn install
unzip $M2_REPO/org/apache/bookkeeper/metadata/drivers/jetcd-core-shaded/4.18.0-SNAPSHOT/jetcd-core-shaded-4.18.0-SNAPSHOT-shaded.jar \
  -d metadata-drivers/jetcd-core-shaded/target/second-classes

# Step 4: Use IntelliJ IDEA's "Compare Directories" tool to compare the `first-classes` 
# and `second-classes` directories. The differences in imports should become apparent.

We can remove the attach and unpack configurations, and it should work fine.

This issue has already been reported in apache/pulsar, and a similar patch has addressed it.

@Shawyeok
Copy link
Contributor Author

There is a CI test failing, investigating on it.

https://github.com/apache/bookkeeper/actions/runs/11870652415/job/33082639394?pr=4526

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM

@Shawyeok Shawyeok force-pushed the fix-jetcd-core-shading-problem branch from 3de9e67 to 6b54df8 Compare November 17, 2024 15:54
@Shawyeok
Copy link
Contributor Author

Shawyeok commented Nov 17, 2024

@lhotari
Typically, a shaded module is intended for external use, but we use jetcd-core-shaded internally in this case.

Currently, Integration Tests are run for the entire metadata-drivers project. After investigating Maven’s artifact resolution during the test lifecycle, it prioritizes resolving artifacts from the current session workspace (including metadata-drivers-parent, jetcd-core-shaded, and metadata-stores-etcd). If not found in the workspace, it falls back to the local repository. In this case, it resolve the artifact of jetcd-core-shaded as /path/to/bookkeeper/metadata-drivers/jetcd-core-shaded/target/classes instead search from local repository.
https://github.com/apache/maven-resolver/blob/5550317b3b035bed4101caf7615ed1f9926263a9/maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultArtifactResolver.java#L349-L379

A reasonable workaround is to exclude jetcd-core-shaded from the integration tests, as it’s a POM-only project with no Java source code.

Subject: [PATCH] Exclude jetcd-core-shaded from metadata driver integration test
---
Index: .github/workflows/bk-ci.yml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/.github/workflows/bk-ci.yml b/.github/workflows/bk-ci.yml
--- a/.github/workflows/bk-ci.yml	(revision b32881139685af9fdb7a6cb06d55e78d8b677257)
+++ b/.github/workflows/bk-ci.yml	(revision 6b54df8f03f82c684827f6b6b80df087f7d33bef)
@@ -242,7 +242,7 @@
         run: mvn -B -nsu clean install -Pdocker -DskipTests
 
       - name: Run metadata driver tests
-        run: mvn -B -nsu -f metadata-drivers/pom.xml test -DintegrationTests
+        run: mvn -B -nsu -f metadata-drivers/pom.xml -pl '!jetcd-core-shaded' test -DintegrationTests
 
       - name: Run all integration tests (except backward compatibility tests)
         run: |

This should able to address CI Integration Tests failures, PTAL.

.github/workflows/bk-ci.yml Show resolved Hide resolved
Copy link
Member

@shoothzj shoothzj left a comment

Choose a reason for hiding this comment

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

What will our future direction be? Should we wait for a new version of jetcd-core?

@Shawyeok
Copy link
Contributor Author

What will our future direction be? Should we wait for a new version of jetcd-core?

@shoothzj
The maintainer of jetcd-core is working on switch to vertx-grpc-client, we could consider change to new version of jetcd-core without shading.

@lhotari
Copy link
Member

lhotari commented Nov 18, 2024

What will our future direction be? Should we wait for a new version of jetcd-core?

@shoothzj We have this shading solution in place until jetcd-core is decoupled from grpc-java. grpc-java is tested with a specific Netty version as documented in https://github.com/grpc/grpc-java/blob/master/SECURITY.md#netty . If a different Netty version is used, things could break. The jetcd-core-shaded solution transforms the jetcd-core library to use grpc-netty-shaded. This PR fixes some issues in the shading configuration.

@shoothzj shoothzj merged commit f119d07 into apache:master Nov 18, 2024
22 of 23 checks passed
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.

4 participants