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][client] Make protobuf-java dependency optional in java client libraries #23632

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

Shawyeok
Copy link
Contributor

@Shawyeok Shawyeok commented Nov 22, 2024

Fixes #23469

Motivation

In the Pulsar Java client libraries, the protobuf-java library is only used in ProtobufSchema, making it suitable as an optional dependency. However, it is mistakenly included in the pulsar-client-admin and pulsar-client-all shaded jar and declared as required dependency of pulsar-client, pulsar-client-original and pulsar-client-admin-original, as noted by @merlimat in this discussion.

PR #23468 moved protobuf-java from a shaded JAR to a declared dependency in the pom.xml, which helped avoid conflicts between protobuf-java and application dependencies. Since users only need protobuf-java when using ProtobufSchema, it makes sense to declare it as an optional dependency. This change reduces the number of dependencies in the Pulsar client libraries.

Verifying this change

  1. Execute the following Maven command:
mvn clean install -am -pl pulsar-client-all,pulsar-client-shaded,pulsar-client-admin-shaded -DskipTests
  1. Verify that the protobuf-java dependency is marked optional in the installed pom.xml files, e.g.
<dependency>
  <groupId>com.google.protobuf</groupId>
  <artifactId>protobuf-java</artifactId>
  <version>3.25.5</version>
  <scope>compile</scope>
  <optional>true</optional>
</dependency>

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#19

Copy link

@Shawyeok Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@github-actions github-actions bot added doc-required Your PR changes impact docs and you will update later. and removed doc-label-missing labels Nov 22, 2024
@lhotari lhotari added this to the 4.1.0 milestone 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. Thanks for addressing this. This could be a minor breaking change for some clients since they would now have to include the protobuf-java version that they prefer. We'll just have to add a release note that this is change is made. Since it's a minor breaking change and not many have upgraded to Pulsar 4.0.x, I think it makes sense to get this included in 4.0.x .

@lhotari lhotari added the release/important-notice The changes which are important should be mentioned in the release note label Nov 26, 2024
@lhotari
Copy link
Member

lhotari commented Nov 26, 2024

Build failure.

https://github.com/apache/pulsar/actions/runs/11977798414/job/3
3566154951?pr=23632#step:11:1

Run src/check-binary-license.sh ./distribution/server/target/apache-pulsar--bin.tar.gz && src/check-binary-license.sh ./distribution/shell/target/apache-pulsar-shell--bin.tar.gz
protobuf-java-3.25.5.jar mentioned in LICENSE, but not bundled

I'd guess that it's the distribution/shell/src/assemble/LICENSE.bin.txt file which should be updated or protobuf-java should be included as a dependency for the shell. I wonder if it's needed there? If not, the LICENSE.bin.txt file should be updated by removing the entry.

@Shawyeok
Copy link
Contributor Author

I'd guess that it's the distribution/shell/src/assemble/LICENSE.bin.txt file which should be updated or protobuf-java should be included as a dependency for the shell. I wonder if it's needed there? If not, the LICENSE.bin.txt file should be updated by removing the entry.

@lhotari

Yes, the protobuf-java dependency should be removed as well.

The protobuf-java dependency in the pulsar-common module was introduced in #17962. It seems unnecessary now, and removing it would be a cleaner approach. Please take a look.

@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.32%. Comparing base (bbc6224) to head (9295b64).
Report is 756 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23632      +/-   ##
============================================
+ Coverage     73.57%   74.32%   +0.75%     
- Complexity    32624    34464    +1840     
============================================
  Files          1877     1944      +67     
  Lines        139502   147202    +7700     
  Branches      15299    16240     +941     
============================================
+ Hits         102638   109408    +6770     
- Misses        28908    29338     +430     
- Partials       7956     8456     +500     
Flag Coverage Δ
inttests 27.20% <ø> (+2.62%) ⬆️
systests 24.32% <ø> (-0.01%) ⬇️
unittests 73.71% <ø> (+0.86%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 667 files with indirect coverage changes

@lhotari lhotari merged commit b284cd4 into apache:master Nov 27, 2024
50 of 51 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
lhotari pushed a commit that referenced this pull request Nov 27, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 28, 2024
…ibraries (apache#23632)

(cherry picked from commit b284cd4)
(cherry picked from commit e1e4296)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Nov 28, 2024
…ibraries (apache#23632)

(cherry picked from commit b284cd4)
(cherry picked from commit e1e4296)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/branch-3.0 cherry-picked/branch-3.3 cherry-picked/branch-4.0 doc-required Your PR changes impact docs and you will update later. release/important-notice The changes which are important should be mentioned in the release note release/3.0.8 release/3.3.3 release/4.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] protobuf-java should be an optional dependency for Pulsar Java client libraries
3 participants