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.

Sign up for GitHub

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

Jump to bottom

[Java] remove guava dependency #1113

Open
chaokunyang opened this issue Nov 20, 2023 · 12 comments
Open

[Java] remove guava dependency #1113

chaokunyang opened this issue Nov 20, 2023 · 12 comments
Labels
enhancement New feature or request java

Comments

@chaokunyang
Copy link
Collaborator

chaokunyang commented Nov 20, 2023 •
edited
Loading

Is your feature request related to a problem? Please describe.

Fury use guava for generic reflection, cache. But guava is used so widely, it's very likely to version conflict with users code.

Describe the solution you'd like

Remove guava dependency, implement all releated methods by fury self.

Here are the things which may need to be finished to make guava optional:

  • Implement a TypeRef to replace TypeToken in guava, which is work that @nandakumar131 doing before.
  • Remove guava Cache which Fury used for weak key and soft values map
  • Remove guava FinalizableWeakReference which fury used to build a multi-key weak map
  • Remove guava ListeningExecutorService which fury used to compile classes and register callback
  • Remove guava com.google.common.collect usage in the src/main/java dir except org.apache.fury.serializer.collection.GuavaCollectionSerializers
  • Make org.apache.fury.serializer.collection.GuavaCollectionSerializers load stub classes, so even guava jar are not in classpath, teh fury init won't throw exception, like we did in org.apache.fury.serializer.collection.ImmutableCollectionSerializers
  • Add a script to ci to check com.google. are only used in GuavaCollectionSerializers and test classes.

Additional context

Shade guava is an option too, but introduce complexities for fury GuavaSerializers and guava CVE
@chaokunyang chaokunyang added enhancement New feature or request java labels Nov 20, 2023
@chaokunyang chaokunyang mentioned this issue Nov 20, 2023
Merged
2 tasks
@nandakumar131
Copy link
Contributor

@chaokunyang
If you haven't started working on the guava removal part 2, I can take it up and post a PR.

@chaokunyang
Copy link
Collaborator Author

chaokunyang commented Dec 22, 2023 •
edited
Loading

Hi @nandakumar131, thanks for the willingness to contribute to Fury. I haven't start part2 yet, you can take up this part work.

FYI, Currently Fury uses guava TypeToken for generics inspection and use guava cache for some static data cache. These are the biggest dependency on guava

@nandakumar131
Copy link
Contributor

Thanks @chaokunyang!
If the change becomes huge, I will split it into two PRs to make it easy to review.

@nandakumar131
Copy link
Contributor

@chaokunyang do we still want to support GuavaCollectionSerializers?

@chaokunyang
Copy link
Collaborator Author

chaokunyang commented Dec 22, 2023 •
edited
Loading

@chaokunyang do we still want to support GuavaCollectionSerializers?

I prefer to still support it, but may use a way without source api dependency. For example, we use methodhandle to get all methods for invocation

@nandakumar131 nandakumar131 mentioned this issue Dec 23, 2023
Merged
2 tasks
chaokunyang pushed a commit that referenced this issue Dec 24, 2023
@nandakumar131
[Java] Remove guava part 2. (#1244)
2112b37 
## What do these changes do?

Remove guava part 2:

Removed guava `Preconditions` usage
Removed guava `Objects` usage
Removed guava `MoreObjects` usage
Removed guava `ImmutableList`
Removed guava `ImmutableSet` & `ImmutableSortedSet`

## Related issue number
#1113 

## Check code requirements

- [ ] tests added / passed (if needed)
- [ ] Ensure all linting tests pass, see
[here](https://github.com/alipay/fury/blob/main/CONTRIBUTING.rst) for
how to run them
@nandakumar131 nandakumar131 mentioned this issue Dec 28, 2023
Closed
@Munoon
Copy link
Contributor

Munoon commented Apr 5, 2024

This also helps in reducing JAR file size. Let me know if I can help you here.

@chaokunyang
Copy link
Collaborator Author

chaokunyang commented Apr 5, 2024 •
edited
Loading

This also helps in reducing JAR file size. Let me know if I can help you here.

@Munoon That would be really great. Actually I am on the work of first release of Fury under asf and don't have enough time for this feature

@Munoon
Copy link
Contributor

Munoon commented Apr 5, 2024

@Munoon That would be really great. Actually I am on the work of first release of Fury under asf and don't have enough time for this feature

Can you please specify which exactly help you need? To complete @nandakumar131 PR or to do some further steps?

@chaokunyang
Copy link
Collaborator Author

chaokunyang commented Apr 6, 2024 •
edited
Loading

@Munoon That would be really great. Actually I am on the work of first release of Fury under asf and don't have enough time for this feature

Can you please specify which exactly help you need? To complete @nandakumar131 PR or to do some further steps?

Here are the things which may need to be finished to make guava optional:

  • Implement a TypeRef to replace TypeToken in guava, which is work that @nandakumar131 doing before.
  • Remove guava Cache which Fury used for weak key and soft values map
  • Remove guava FinalizableWeakReference which fury used to build a multi-key weak map
  • Remove guava ListeningExecutorService which fury used to compile classes and register callback
  • Remove guava com.google.common.collect usage in the src/main/java dir except org.apache.fury.serializer.collection.GuavaCollectionSerializers
  • Make org.apache.fury.serializer.collection.GuavaCollectionSerializers load stub classes, so even guava jar are not in classpath, teh fury init won't throw exception, like we did in org.apache.fury.serializer.collection.ImmutableCollectionSerializers
  • Add a script to ci to check com.google. are only used in GuavaCollectionSerializers and test classes.

@Munoon
Copy link
Contributor

Munoon commented Apr 6, 2024

OK, I'll separate those by different PRs starting from TypeRef implementation, which looks like the most hard one, so it'll take some time...

@chaokunyang
Copy link
Collaborator Author

OK, I'll separate those by different PRs starting from TypeRef implementation, which looks like the most hard one, so it'll take some time...

Yep, the TypeRef is the most compilciated one, and the TypeToken in guava has some performance issue, so we did some optimization in org.apache.fury.type.TypeUtils. If we have our own implementation, such helper can be simplified a lot.

@nandakumar131
Copy link
Contributor

@chaokunyang, sorry about the delay on #1273.
I will not be able to make any progress on the PR in next two weeks, I can take it up after that.

@Munoon, feel free to take over #1273, if you have started working on TypeToken changes.

@Munoon Munoon mentioned this issue Apr 21, 2024
Merged
chaokunyang pushed a commit that referenced this issue Apr 26, 2024
@Munoon @nandakumar131
refactor(java): replace Guava's TypeToken with self-made (#1553)
a003624 

## What does this PR do?

Removes Guava's `TypeToken` usages and replaces it with a hand-made
`TypeToken` implementation.
To be honest, this is mostly a copy-paste of Guava's TypeToken with
removing some unnecessary stuff. This implementation can be improved and
optimized.

## Related issues

#1113
#1273

---------

Co-authored-by: Nandakumar Vadivelu <nanda@apache.org>
@Munoon Munoon mentioned this issue May 7, 2024
Merged
2 tasks
chaokunyang pushed a commit that referenced this issue May 8, 2024
@Munoon
refactor(java): Remove Guava's Collection usages (#1611)
a87b902 
## What does this PR do?

Remove Guava's Collection usages

## Related issues

<!--
Is there any related issue? Please attach here.

- #xxxx0
- #xxxx1
- #xxxx2
-->
#1113

## Does this PR introduce any user-facing change?

<!--
If any user-facing interface changes, please [open an
issue](https://github.com/apache/incubator-fury/issues/new/choose)
describing the need to do so and update the document if necessary.
-->

- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?

Lazy map become slightly more lazy =)

<!--
When the PR has an impact on performance (if you don't know whether the
PR will have an impact on performance, you can submit the PR first, and
if it will have impact on performance, the code reviewer will explain
it), be sure to attach a benchmark data here.
-->
@Munoon Munoon mentioned this issue May 8, 2024
Merged
2 tasks
chaokunyang pushed a commit that referenced this issue May 8, 2024
@Munoon
refactor(java): Remove Guava's Concurrency utils usages (#1614)
b2fdd68 
<!--
**Thanks for contributing to Fury.**

**If this is your first time opening a PR on fury, you can refer to
[CONTRIBUTING.md](https://github.com/apache/incubator-fury/blob/main/CONTRIBUTING.md).**

Contribution Checklist

- The **Apache Fury (incubating)** community has restrictions on the
naming of pr titles. You can also find instructions in
[CONTRIBUTING.md](https://github.com/apache/incubator-fury/blob/main/CONTRIBUTING.md).

- Fury has a strong focus on performance. If the PR you submit will have
an impact on performance, please benchmark it first and provide the
benchmark result here.
-->

## What does this PR do?

Removes Guava's Concurrency utils usages

## Related issues

#1113


## Does this PR introduce any user-facing change?

<!--
If any user-facing interface changes, please [open an
issue](https://github.com/apache/incubator-fury/issues/new/choose)
describing the need to do so and update the document if necessary.
-->

- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?




<!--
When the PR has an impact on performance (if you don't know whether the
PR will have an impact on performance, you can submit the PR first, and
if it will have impact on performance, the code reviewer will explain
it), be sure to attach a benchmark data here.
-->
@Munoon Munoon mentioned this issue May 11, 2024
Merged
2 tasks
chaokunyang pushed a commit that referenced this issue May 13, 2024
@Munoon
refactor(java): replace Guava's string utility methods with own imple…
9897574 
…mentation (#1624)

## What does this PR do?

<!-- Describe the purpose of this PR. -->


## Related issues

<!--
Is there any related issue? Please attach here.

- #xxxx0
- #xxxx1
- #xxxx2
-->
#1113

## Does this PR introduce any user-facing change?

<!--
If any user-facing interface changes, please [open an
issue](https://github.com/apache/incubator-fury/issues/new/choose)
describing the need to do so and update the document if necessary.
-->

- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?
@chaokunyang chaokunyang mentioned this issue May 20, 2024
Merged
2 tasks
@chaokunyang chaokunyang mentioned this issue Jul 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request java
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nandakumar131 @chaokunyang @Munoon