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

Remove guava from data-avro. #409

Merged
merged 4 commits into from
Dec 2, 2021
Merged

Remove guava from data-avro. #409

merged 4 commits into from
Dec 2, 2021

Conversation

tjni
Copy link
Contributor

@tjni tjni commented Sep 7, 2020

The only usage of Guava in data-avro is the CaseFormat class, which is
used to convert from lower case camel-case to upper case camel-case. I
don't think it justifies pulling in the entire library (which is 2.1 MB
in version 18 and 2.6 MB in version 28.1 plus around 1.5 MB of source).

The CaseFormat class has some dependencies, including one on CaseMatcher
that isn't easy to remove, so I've copied over that code from the latest
version of Guava for now (manually shading them) and updated a minimum
amount of code to remove other dependencies on Preconditions, Converter,
and annotations.

Note: Please add the backwards-incompatible label.

@tjni tjni marked this pull request as ready for review September 7, 2020 19:55
@evanw555 evanw555 added the backward-incompatible Changes/removes an existing API, requires major version bump. PRs with this label should be bundled. label Sep 9, 2020
@evanw555
Copy link
Contributor

evanw555 commented Sep 9, 2020

This is good. There was an effort in the past to remove Guava completely from Rest.li, since supposedly Guava often causes transitive dependency problems. It was left in a couple complex cases, this being one of them, however the intention was to completely remove it. This is a step in the right direction.

As for where to place the shadowed code, that's a little tricky. In the past, we've included shadowed code in separate modules such as li-jersey-uri and li-protobuf, however adding a whole new module adds complexity. I'm not too sure about this. It would be helpful to get another opinion here.

@tjni
Copy link
Contributor Author

tjni commented Sep 9, 2020

Thanks for reviewing! What you wrote makes sense. I don't mind either way (and don't have a strong opinion either).

The only usage of Guava in data-avro is the CaseFormat class, which is
used to convert from lower case camel-case to upper case camel-case. I
don't think it justifies pulling in the entire library (which is 2.1 MB
in version 18 and 2.6 MB in version 28.1 plus around 1.5 MB of source).

The CaseFormat class has some dependencies, including one on CaseMatcher
that isn't easy to remove, so I've copied over that code from the latest
version of Guava for now (manually shading them) and updated a minimum
amount of code to remove other dependencies on Preconditions, Converter,
and annotations.
@tjni
Copy link
Contributor Author

tjni commented Sep 10, 2020

I updated the code to remove usage of CaseFormat and reused the code from Guava to transform a character from lowercase to uppercase.

Copy link
Contributor

@evanw555 evanw555 left a comment

Choose a reason for hiding this comment

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

Glad we're able to solve this now without shadowing any Guava source files 😄

LGTM, we'll bundle this with the next major version release

@evanw555 evanw555 added the v30 Should be bundled with the version 30.0.0 release label Oct 27, 2021
@nickibi nickibi changed the base branch from master to release/v30 December 2, 2021 22:38
@nickibi nickibi merged commit 1aa7b58 into linkedin:release/v30 Dec 2, 2021
evanw555 pushed a commit that referenced this pull request Dec 3, 2021
The only usage of Guava in data-avro is the CaseFormat class, which is
used to convert from lower case camel-case to upper case camel-case. I
don't think it justifies pulling in the entire library (which is 2.1 MB
in version 18 and 2.6 MB in version 28.1 plus around 1.5 MB of source).

The CaseFormat class has some dependencies, including one on CaseMatcher
that isn't easy to remove, so I've copied over that code from the latest
version of Guava for now (manually shading them) and updated a minimum
amount of code to remove other dependencies on Preconditions, Converter,
and annotations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backward-incompatible Changes/removes an existing API, requires major version bump. PRs with this label should be bundled. v30 Should be bundled with the version 30.0.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants