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

[improve][broker] Include runtime dependencies in server distribution #22001

Conversation

dragosvictor
Copy link
Contributor

@dragosvictor dragosvictor commented Jan 31, 2024

Motivation

The server distribution assembly does not currently include artifact runtime dependencies, pulling only the compile dependencies instead. While this has not impacted the customers, a class loading issue can always crop up. This is what happened while importing OpenTelemetry dependencies related to PIP-320. OpenTelemetry itself marks its many dependencies as runtime (see, for instance, https://mvnrepository.com/artifact/io.opentelemetry/opentelemetry-exporter-otlp/1.34.1). Including each of them manually in our POM is both tedious and error-prone.

Modifications

  • Reverts the dependency scope for the server assembly to the default value, runtime.
  • Manually excludes some of the libraries that would be needlessly pulled.
  • Allows org.roaringbitmap-shims-0.9.44.jar to be included in the assembly.
  • Removes assembly exclusion for lombok as it is not needed anymore.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is already covered by existing integration tests.

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: dragosvictor#9

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jan 31, 2024
@dragosvictor dragosvictor marked this pull request as ready for review January 31, 2024 05:08
@merlimat merlimat added this to the 3.3.0 milestone Jan 31, 2024
@asafm
Copy link
Contributor

asafm commented Jan 31, 2024

Perhaps we should double-check by making a diff between the included JARs in the before and after versions?

@merlimat
Copy link
Contributor

This is actually done by the license checked script :) The list there needs to match the jars included

@dragosvictor
Copy link
Contributor Author

Perhaps we should double-check by making a diff between the included JARs in the before and after versions?

@merlimat pointed out that the license check essentially achieves this already. For extra caution, here's the assembly lib/ diff between the master branch and the proposed changes:

145d144
< javax.annotation-javax.annotation-api-1.3.2.jar
296a296
> org.roaringbitmap-shims-0.9.44.jar

@merlimat merlimat merged commit 57025bc into apache:master Jan 31, 2024
48 of 49 checks passed
@dragosvictor dragosvictor deleted the fix-pulsar-distribution-lib-dependency-scope branch January 31, 2024 19:45
lhotari pushed a commit that referenced this pull request Jun 13, 2024
lhotari pushed a commit that referenced this pull request Jun 13, 2024
lhotari pushed a commit that referenced this pull request Jun 13, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 17, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 17, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 25, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 25, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 1, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 1, 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.

4 participants