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

replace usage of guava with standard java #1208

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

askoog
Copy link
Contributor

@askoog askoog commented Aug 24, 2023

Avoid adding extra third party dependency 'guava' by replacing methods with equivalent alternatives in standard jdk.

This helps users of etcd since every transitive third party dependency requires more code to be downloaded and might result in dependency conflicts.

fixes #1207

Copy link
Contributor

@liangyuanpeng liangyuanpeng left a comment

Choose a reason for hiding this comment

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

make sense for me

@lburgazzoli
Copy link
Collaborator

@askoog does this actually remove guava dependency ? I think this is still something that is transitively provided by grpc-java

@liangyuanpeng
Copy link
Contributor

I agree with reducing direct dependencies even though indirect dependencies may exist.

@lburgazzoli
Copy link
Collaborator

the reason guava dependency is explicitly declared was to control it as unfortunately there were cases where guava was clashing anyway, but we can remove it and see how it goes.

Unfortunately it does not prevent to break apps using jetcd as a library.

@lburgazzoli
Copy link
Collaborator

@askoog @liangyuanpeng I'm not very convinced by removing the explicit dependency as guava is anyway leveraged by jetcd so it is a good practice to explicit defines dependencies the application depends on instead of rely on transitive dependencies as any change in the transitive dependency chain can break the app.

So I'm fine to replace guava with standard java code but not to remove the dependency as long as jetcd still requires it.

WDYT ?

@askoog
Copy link
Contributor Author

askoog commented Aug 24, 2023

wow, you guys are fast! Thanks for the quick response!

You are right @lburgazzoli, this does not actually remove the dependency since it's also transitively included. And I agree with you that you should declare the versions of your used transitive dependencies as well.

The problem we were trying to solve initially was that in some of our legacy applications we were bound to guava 20 which grpc seems to work fine with, but the usage of the Streams class in jetcd was not compatible with such an old version. So by removing the usage of guava code directly but keeping the specified dependency version will work for us. I will revert the changes in the libs file so that the version is still specified there

@lburgazzoli
Copy link
Collaborator

if you find out other guava bits that can be replaced by any standard java class, I'm more than happy.

In fact one of the reason we have some additional wrappers was exactly to avoid exposing guava as much as possible at well, that is a pain :)

@lburgazzoli
Copy link
Collaborator

I don't see any build file with libs.guava as dependency, I think the modules where guava is required should explicit declare the guava dependency

@lburgazzoli
Copy link
Collaborator

@askoog any update ?

@lburgazzoli
Copy link
Collaborator

@askoog can you please rebase / fix conflicts ?

@lburgazzoli lburgazzoli force-pushed the remove-guava-dependency branch 2 times, most recently from 5f86725 to 0a1412f Compare October 16, 2023 10:00
Avoid adding extra third party dependency 'guava' by replacing methods with equivalent alternatives in standard jdk.

This helps users of etcd since every transitive third party dependency requires more code to be downloaded and might result in dependency conflicts.

fixes etcd-io#1207

Signed-off-by: Andreas Skoog <andreas.skoog@avanza.se>
@lburgazzoli lburgazzoli force-pushed the remove-guava-dependency branch 3 times, most recently from f2d36c6 to 939341a Compare October 16, 2023 10:41
Signed-off-by: Luca Burgazzoli <lburgazzoli@gmail.com>
@lburgazzoli lburgazzoli merged commit 9c44c72 into etcd-io:main Oct 16, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Remove guava dependency
3 participants