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

request: return ListenableFutures in PubSub async methods rather than bare Futures #1148

Closed
mattnworb opened this issue Aug 10, 2016 · 3 comments · Fixed by #1168
Closed
Assignees

Comments

@mattnworb
Copy link

The com.google.cloud.pubsub.PubSub interface defines async operations which return java.util.concurrent.Future values.

For anyone who uses the ListenableFuture concept from Guava (or CompletableFutures in Java 8), these Futures have to be adapted into ListenableFutures by spawning a thread that blocks on the Future.get() return.

It would be much more convenient if the Futures returned from PubSub were actually ListenableFuture instances (even if the interface did not define the return value to be ListenableFuture) - then it would not be necessary for users to block one thread per future they wish to adapt into a ListenableFuture.

For instance, internally PubSubImpl.pullAsync(final String subscription, int maxMessages) is implemented by using the listenable-like com.google.cloud.pubsub.spi.PubSubRpc.PullFuture and attaching callbacks to it, but the returned Future is an anonymous subclass of Future created by Futures.lazyTransform(..).

Is it a conscious decision to avoid using ListenableFutures in the interfaces defined in gcloud-java-pubsub and other clients?

It seems like the com.google.cloud.pubsub.spi layer has access to ListenableFuture instances via the generated grpc client code (like subscriberApi.acknowledgeCallable().futureCall(request)) but then these are discarded in the PubSubImpl layer.

@mziccard
Copy link
Contributor

Is it a conscious decision to avoid using ListenableFutures in the interfaces defined in gcloud-java-pubsub and other clients?

Yes, it's a conscious decision. We don't want to expose Guava-related classes in our interfaces and we still want to support Java 7 (for GAE and Android, mainly).

Could MessageConsumer pullAsync(String subscription, MessageProcessor callback, PullOption... options) help? If not, what is it missing that you need (I would be happy to add more features)?

@mattnworb
Copy link
Author

Thanks @mziccard for the fast reply.

MessageConsumer is great - it perfectly fits my needs for consuming messages. It is awesome to not have to implement something that pulls messages in a loop and processes them in our own code.

The (minor) pain point comes more in code that deals with methods like Future<String> publishAsync(String topic, Message msg), when I would like to be able to transform the returned future to some other type or combine it with another future. We are heavy users of ListenableFuture and find it natural to transform and otherwise chain ListenableFutures together, so it is a small annoyance to have to use JdkFutureAdapters.listenInPoolThread and create a thread that blocks on Future.get() to be able to adapt this response to a ListenableFuture.

I understand not wanting to return Guava types in the PubSub interface, but wouldn't it be possible to return ListenableFuture implementations (SettableFuture, etc) in the PubSub implementation? This way adapters like JdkFutureAdapters would avoid having to spawn a blocked thread since the returned future would pass a if (future instanceof ListenableFuture) ... check.

@mziccard
Copy link
Contributor

I understand not wanting to return Guava types in the PubSub interface, but wouldn't it be possible to return ListenableFuture implementations (SettableFuture, etc) in the PubSub implementation?

I will look into this, I'll keep you posted!

github-actions bot pushed a commit to suztomo/google-cloud-java that referenced this issue Jun 29, 2022
…#1148) (googleapis#544)

After discussion with OSPO, it has been decided to remove "All rights reserved" clause from the java.header file to better reflect the [correct license header](https://g3doc.corp.google.com/company/teams/opensource/releasing/preparing.md?cl=head#Apache-header).
Source-Link: googleapis/synthtool@09c59c2
Post-Processor: gcr.io/repo-automation-bots/owlbot-java:latest@sha256:2e88a4a7fe3377cf8de1fa5982134f6ef2768980fa2f94edcc1ba6604ae2e7ca
github-actions bot pushed a commit that referenced this issue Jul 1, 2022
After discussion with OSPO, it has been decided to remove "All rights reserved" clause from the java.header file to better reflect the [correct license header](https://g3doc.corp.google.com/company/teams/opensource/releasing/preparing.md?cl=head#Apache-header).
Source-Link: googleapis/synthtool@09c59c2
Post-Processor: gcr.io/repo-automation-bots/owlbot-java:latest@sha256:2e88a4a7fe3377cf8de1fa5982134f6ef2768980fa2f94edcc1ba6604ae2e7ca
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 a pull request may close this issue.

2 participants