Skip to content

Issue-409 BookKeeper Server distribution contains useless jar guava#687

Closed
eolivelli wants to merge 1 commit intoapache:masterfrom
eolivelli:issue-409-no-guava
Closed

Issue-409 BookKeeper Server distribution contains useless jar guava#687
eolivelli wants to merge 1 commit intoapache:masterfrom
eolivelli:issue-409-no-guava

Conversation

@eolivelli
Copy link
Copy Markdown
Contributor

Exclude Guava Jar from assembly configuration. Guava is still present by relocated

@eolivelli eolivelli requested a review from jiazhai November 3, 2017 07:54
@eolivelli
Copy link
Copy Markdown
Contributor Author

This change affects bookkeeper-server and bookkeeper-dist

@asfgit
Copy link
Copy Markdown

asfgit commented Nov 3, 2017

SUCCESS

--none--

Copy link
Copy Markdown
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

actually -- have you verified if this works for bookkeeper-dist? I suspect it doesn't work for bookkeeper-dist, because bookkeeper-dist includes other plugins which might need guava package.

Copy link
Copy Markdown
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

I'd like to hold this, before we verify my comment.

@sijie
Copy link
Copy Markdown
Member

sijie commented Nov 10, 2017

@eolivelli if you run mvn dependency:tree, you will find guava is used at multiple modules (e.g. bookkeeper-benchmark, almost all the stats provider implementations). so your change doesn't work for bookkeeper-dist package.

@eolivelli
Copy link
Copy Markdown
Contributor Author

after all I think that we can close this issue as won't fix.
it is not worth to complicate the build. Guava is still on classpath but shaded on minimal bookie distribution.
ok @sijie ?

@sijie
Copy link
Copy Markdown
Member

sijie commented Nov 10, 2017

sure let me close it. mark it as "won't fix"

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