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][client] Exclude log4j-slf4j-impl from compile dep in pulsar-client-all #19937

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

tisonkun
Copy link
Member

This closes #19484.

The root cause may be apache/bookkeeper@f0988dd#r106236889 and we bump to affected BK version in Pulsar 2.11.

Motivation

Avoid pull in log4j-slf4j-impl in pulsar-client-all.

Modifications

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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:

@tisonkun tisonkun self-assigned this Mar 27, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 27, 2023
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

good catch

would you fix it also in bookkeeper ?
there is an ongoing VOTE for 4.16.0

@tisonkun
Copy link
Member Author

would you fix it also in bookkeeper ?

I'm glad to do so. But updating dependencies can be tricky. I have a conversation with @Shoothzj and he has some ideas to improve the overall situation. I'll wait for his updates.

Anyway, file an issue at apache/bookkeeper#3891

@hezhangjian
Copy link
Member

sorry for my late reply. @eolivelli @tisonkun I have submit a PR for review apache/bookkeeper#3892.
I verified the module doesn't contains log4j dependency. And the server package contains it.

@tisonkun
Copy link
Member Author

Merging..

After we upgrade to BK 4.16.0, we can revisit this issue and do some cleanups.

@tisonkun tisonkun merged commit d14e43e into apache:master Mar 28, 2023
tisonkun added a commit that referenced this pull request Mar 28, 2023
…-client-all (#19937)

(cherry picked from commit d14e43e)

Ref - #19484

Signed-off-by: tison <[email protected]>
@tisonkun tisonkun deleted the exclude-log4j-impl branch March 28, 2023 03:36
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.

[Bug] pulsar-client-all 2.11.0 transitively bringing in log4j2 implementation
3 participants