Skip to content

Commit

Permalink
Fix potential jetcd-core shading problem (#4526)
Browse files Browse the repository at this point in the history
### 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][1] and in the [grpc-java documentation][2].

[1]: #4426 (comment)
[2]: https://github.com/grpc/grpc-java/blob/master/SECURITY.md#netty

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:
```shell
# 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][3] in apache/pulsar, and a similar [patch][patch] has addressed it.

[3]: apache/pulsar#23513
[patch]: apache/pulsar#23604
  • Loading branch information
Shawyeok authored Nov 18, 2024
1 parent 0df2240 commit f119d07
Show file tree
Hide file tree
Showing 3 changed files with 3 additions and 50 deletions.
4 changes: 3 additions & 1 deletion .github/workflows/bk-ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,9 @@ jobs:
run: mvn -B -nsu clean install -Pdocker -DskipTests

- name: Run metadata driver tests
run: mvn -B -nsu -f metadata-drivers/pom.xml test -DintegrationTests
# Exclude jetcd-core-shaded from integration tests, as it’s a POM-only project used internally,
# and maven prioritizes workspace artifacts during testing.
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: |
Expand Down
1 change: 0 additions & 1 deletion metadata-drivers/etcd/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
<groupId>org.apache.bookkeeper.metadata.drivers</groupId>
<artifactId>jetcd-core-shaded</artifactId>
<version>${project.version}</version>
<classifier>shaded</classifier>
<exclusions>
<exclusion>
<groupId>io.etcd</groupId>
Expand Down
48 changes: 0 additions & 48 deletions metadata-drivers/jetcd-core-shaded/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -132,54 +132,6 @@
<file>${project.basedir}/dependency-reduced-pom.xml</file>
</transformer>
</transformers>
<!-- required for IntelliJ support -->
<shadedArtifactAttached>true</shadedArtifactAttached>
<shadedClassifierName>shaded</shadedClassifierName>
</configuration>
</execution>
</executions>
</plugin>
<!-- required for IntelliJ support, for some reason shadedArtifactAttached isn't sufficient alone -->
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>build-helper-maven-plugin</artifactId>
<executions>
<execution>
<id>attach-shaded-jar</id>
<phase>package</phase>
<goals>
<goal>attach-artifact</goal>
</goals>
<configuration>
<artifacts>
<artifact>
<file>${project.build.directory}/${project.artifactId}-${project.version}-shaded.jar</file>
<type>jar</type>
<classifier>shaded</classifier>
</artifact>
</artifacts>
</configuration>
</execution>
</executions>
</plugin>
<!-- required for running tests with "mvn -f metadata-drivers/pom.xml test -DintegrationTests" -->
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-antrun-plugin</artifactId>
<version>${maven-antrun-plugin.version}</version>
<executions>
<execution>
<id>unpack-shaded-jar</id>
<phase>package</phase>
<goals>
<goal>run</goal>
</goals>
<configuration>
<target>
<unzip src="${project.build.directory}/${project.artifactId}-${project.version}-shaded.jar"
dest="${project.build.outputDirectory}"
overwrite="true" />
</target>
</configuration>
</execution>
</executions>
Expand Down

0 comments on commit f119d07

Please sign in to comment.