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][misc] Class conflict during jetcd-core-shaded shading process #23641

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

Shawyeok
Copy link
Contributor

@Shawyeok Shawyeok commented Nov 26, 2024

Motivation

In the latest release candidates (3.0.8-candidate-1, 4.0.1-candidate-1), the jetcd-core-shaded module shading issue persists. This occurs because the maven-shade-plugin includes the current project artifact in the shade process. In jetcd-core-shaded, we copy io.etcd.jetcd.* into the jetcd-core-shaded jar without relocation. As a result, when the install or deploy goals are executed without cleaning the target folder, class conflicts arise between io.etcd.jetcd.* inside org.apache.pulsar:jetcd-core-shaded.jar and io.etcd:jetcd-core.jar.

#23604 attempted to resolve this issue but failed to account for build and deploy in separate steps during the release process.

Verifying this Change

To verify this patch, follow these steps:

# Step 1: Clean and install the `jetcd-core-shaded` module.
mvn clean install -pl jetcd-core-shaded -DskipTests

# Step 2: Install without cleaning, simulating a release process.
mvn install -pl jetcd-core-shaded -DskipTests

# Step 3: Unpack the shaded jar into a directory and verify imports.
# Ensure that the import statement for `io.netty.handler.logging.ByteBufFormat` 
# in `org/apache/pulsar/jetcd/shaded/io/vertx/core/net/NetClientOptions.class` 
# is correct. The expected import is:
# `import io.grpc.netty.shaded.io.netty.handler.logging.ByteBufFormat;`.
unzip $M2_REPO/org/apache/pulsar/jetcd-core-shaded/<VERSION>/jetcd-core-shaded-<VERSION>-shaded.jar \
  -d jetcd-core-shaded/target/shaded-classes

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: Shawyeok#20

… to execute shade multiple times without clean
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 26, 2024
@Shawyeok Shawyeok changed the title [fix][misc] Adjust jetcd-core-shaded shading configuration to be able to execute shade multiple times without clean [fix][misc] Class conflict during jetcd-core-shaded shading process Nov 26, 2024
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. Good work @Shawyeok. Thanks for finding the root cause and addressing the problem.

@lhotari
Copy link
Member

lhotari commented Nov 26, 2024

@Shawyeok Please submit a similar PR to BookKeeper for jetcd-core-shaded.

@lhotari lhotari merged commit 3e108da into apache:master Nov 26, 2024
55 of 56 checks passed
lhotari pushed a commit that referenced this pull request Nov 27, 2024
lhotari pushed a commit that referenced this pull request Nov 27, 2024
@Shawyeok Shawyeok deleted the fix-jetcd-core-shading-issue-2 branch November 27, 2024 14:56
lhotari pushed a commit that referenced this pull request Nov 27, 2024
@Shawyeok
Copy link
Contributor Author

Shawyeok commented Nov 28, 2024

@Shawyeok Please submit a similar PR to BookKeeper for jetcd-core-shaded.

@lhotari
apache/bookkeeper#4532 please feel free to take a look, thanks.

nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 28, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants