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

feat: use java 8 api instead of exposing guava types #30

Closed
wants to merge 1 commit into from

Conversation

Nowheresly
Copy link

Hi qdrant team.

Well, this is quite a big PR for a first contribution... all the more so as it introduces a breaking change.
And without even starting a discussion before proposing this PR.
So first of all, I apologize for that.

Why this PR?

Currently, the QdrantClient exposes ListenableFuture guava type in its public api. Prior to java 8, the guava library was the only way to implement asynchronous programming in java. Starting with java 8, CompletionStage and CompletableFuture have been added for that purpose.

Usually, ListenableFuture is used when java 6/7 compatibility is required. Since Qdrant java-client requires java 8 minimum, there's nothing preventing the use of CompletableFuture.

I have spent so much time migrating code base from ListenableFuture to CompletableFuture... I don't want to migrate yet another code base just because a new library exposing ListenableFuture is added. So I started to write a wrapper around QdrantClient to get rid of ListenableFuture.

The goal of this PR is to remove the usage of ListenableFuture in the public api of this library. So I can avoid using a wrapper (good for me!) and you will avoid exposing thirdparty library to your public api (good for you!).

Why not just sticking with ListenableFuture?

ListenableFuture tends to be less and less used in favor of CompletableFuture. At any time, the guava team may decide to move forward and uses the official api. This change may never happen admittedly, yet the official guava documentation is mentioning this eventuality.

For example, the datastax cassandra client library moves from ListenableFuture to CompletableFuture (version 3 is using ListenableFuture while version 4 uses CompletableFuture).

The more we wait, the more painful the migration will be.

How the migration have been done in this PR?

Since grpc is using ListenableFuture, we have to convert ListenableFuture to CompletableFuture. The easiest way to do that is to use the future-converter library.

Now, the drawbacks of using this library are the following:

  • usually it's better to not depend on new external library
  • this library is now archived (no more updates!!)
  • the amount of code needed for the conversion is rather small

To avoid these drawbacks, I have copied and paste code from future-converter in a sub-package. Fortunately, it's the same license!

For the other parts of the migration, I used this guide.

Please not that the guava library is still required at runtime to use your lib. Yet, it is no more exposed in the public api. So guava could be (hypothetically) safely removed without harm in a future version.

What if this PR is rejected?

It's perfectly fine, no drama! Introducing a breaking change is always a hard choice.

I would like to avoid breaking change

Maybe other strategies can be considered, such as adding a new QdrantClientJava8 while keeping the current one to maintain backwards compatibility (or to ease the migration). This comes with the drawback of having to maintain two versions of the code.

At least, we could use this PR to discuss alternatives.

Thanks for taking the time to read this PR.

@Anush008
Copy link
Member

Hi @Nowheresly. Thanks for your effort and explanation.

Since there's quite a lot of change, which is also breaking, we'd only want to have this if there's a necessity. From

At any time, the guava team may decide to move forward and uses the official api. This change may never happen admittedly, yet the official guava documentation is mentioning this eventuality.

that doesn't seem to be the case.

I would prefer to migrate if and when grpc-java moves to CompletableFuture. The implementation would be simpler. We could do this with a new client class, eventually deprecating the previous.

I'll also try to get @russcam in the loop here.

Thanks again. We'll see how this goes. If Russ presents his opinion, we can then go with the most favoured one.

@Anush008 Anush008 requested review from Anush008 and russcam April 20, 2024 18:54
@russcam
Copy link
Collaborator

russcam commented Apr 23, 2024

@Nowheresly Thank you for opening this PR and laying out the rationale.

I'm in agreeance with @Anush008; an ideal time to look to move to CompletableFuture<T> would be when grpc-java moves to generating stubs with it instead of ListenableFuture<T>. Both the underlying "low-level" gRPC client and the "high-level" client would then have consistent API method signatures, which I think is useful to have, especially now the gRPC client is exposed on the high level.

@Nowheresly
Copy link
Author

Thanks @Anush008 and @russcam for taking time to review my PR.
As I said previously, no drama :)

@Anush008
Copy link
Member

Opened an issue for tracking.

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