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

Initial effort to remove Guava dependency. #137

Closed
wants to merge 4 commits into from
Closed

Conversation

thorbjornsen
Copy link

Replaced all references to Guava Optional with java.util.Optional. There were a few cases where direct translation/support is not available in Java 1.8 Optional class (or at all). java.util.Optional is not serializable, etc.

Note that there were two unit tests that were failing prior to the changes.

Eric Thorbjornsen added 2 commits July 30, 2019 16:00
@xerial
Copy link
Member

xerial commented Jul 31, 2019

Thanks. This is a good first step. The next will be removing ImmutableList.of(...) usages

A key concern is serializing Optional.

I didn't know Optional in Java8 is non-serializable. It means Guava's Optional is much better for the current use cases in td-client-java. For example,td-spark needs to send table schema objects with Optional fields to remote worker nodes. So Optional fields must be serializable.

@xerial
Copy link
Member

xerial commented Jul 31, 2019

Spark is implementing their own Optional class. I think we should do a similar thing: https://github.com/apache/spark/blob/master/core/src/main/java/org/apache/spark/api/java/Optional.java

Instead of using java8's optional, we should implement our own like com.treasuredata.client.Optional

@thorbjornsen
Copy link
Author

Per discussion here https://treasure-data.slack.com/archives/C03JNT0A6/p1564587329452600

Only two model classes have implemented the Serializable interface, TDColumn and TDColumnType. I improved the unit tests to ensure that the classes serialize and deserialize correctly. No need to overcomplicate things by implementing our own Optional class.

Improved the serialization unit test by serializing an object, the deserializing the result and comparing the original object to the deserializaed objet. They must be the same.
@thorbjornsen thorbjornsen requested a review from xerial July 31, 2019 23:03
@xerial
Copy link
Member

xerial commented Jan 25, 2023

Superseded by #181

@xerial xerial closed this Jan 25, 2023
@exoego exoego deleted the remove-guava branch January 27, 2023 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants