Replace Pub/Sub Subscriber-based implementation with Pull-based

[meltsufin]

Pub/Sub streaming subscriber API is currently not well suited for low QPS applications.

See #984 for more context.
See docs for an explanation.
cc: @kir-titievsky

[dhoard]

@meltsufin is the plan to keep the BasicAcknowledgeablePubsubMessage and AcknowledgeablePubsubMessage classes?

[meltsufin]

We will be able to collapse them. Basic won't be needed anymore because it comes from the constraints of the streaming API.

[dhoard]

Thoughts on introducing PubSubPullerOperations / PubSubPullerTemplate for pull methods ... with PubSubSubscriberOperations / PubSubSubscriberTemplate being subscribe / asynchronous oriented?

The asynchronous PubSubSubscriberOperations / PubSubSubscriberTemplate has configuration - pull thread count, queue size, outstanding messages, ack mode, etc. which could be set via a methods or via a PubSubSubscriberOperations.Options object.

These really don't apply to usage of the pull methods.

[meltsufin]

We were thinking of just replacing the PubSubSubscriberOperations implementation with the pull-based one, which you are calling PubSubPullerOperations. The settings that no longer make sense would need to be removed as well as part of it.

We can also just add PubSubPullerOperations and at the same time just deprecate PubSubSubscriberOperations. That could make sense.

Regardless of that, I think we need to change PubSubTemplate to use the pull-based approach for subscribing.

[artembilan]

That's what I was talking in the beginning: the template approach should be free from any stateful operations.
The pull approach is OK here and it is fully aligned with what we have in the RabbitTemplate.receive() and similar "pull"-like operations in other templates.

The subscribe logic should be moved to the SubscriberFactory and/or new components to handle subscriptions and acknowledgement.

Does it make sense?

[dhoard]

@artembilan agreed.

[meltsufin]

Makes sense to me too. @elefeint is planning to start work on this later this quarter.

[elefeint]

@artembilan @meltsufin what do you think about the plan below?

Part I, make it easy to bypass PubSub async pull's message hoarding behavior when using GCP-to-SpringIntegration and GCP-to-CloudStream integrations.

• Create new PubSubPollingChannelAdapter, extending SourcePollingChannelAdapter.
  • Implement SourcePollingChannelAdapter::receiveMessage using PubSubSubscriberTemplate::pull.
  • Document tradeoffs between SourcePollingChannelAdapter and PubSubInboundChannelAdapter.

Part II, simplify PubSubSubscriberTemplate (should probably be a separate tracking bug)

• Rename PubSubInboundChannelAdapter to PubSubSubscribingChannelAdapter
• Switch PubSubSubscribingChannelAdapter from using PubSubSubscriberOperations:subscribeAndConvert to using SubscriberFactory::createSubscriber directly.
• Deprecate PubSubSubscriberTemplate::subscribe, ::subscribeAndConvert.
• Update documentation to recommend using either Spring Integration or SubscriberFactory directly when async pull is required.

Part III, message conversion

• Autoconfigure bean pubsubMessageChannelMessageConverter or provide @Transformer (based on type? or on header?)
• Update documentation to recommend using Spring Integration with org.springframework.messaging.converter.MessageConverter implementations instead of pullAndConvert/subscribeAndConvert.

[artembilan]

No, you can't extend SourcePollingChannelAdapter. you must think to implement a AbstractMessageSource. Therefore PubSubMessageSource.
Something similar with have in Spring Integration with the AmqpMessageSource: https://github.com/spring-projects/spring-integration/blob/master/spring-integration-amqp/src/main/java/org/springframework/integration/amqp/inbound/AmqpMessageSource.java
And with the KafkaMessageSource: https://github.com/spring-projects/spring-integration-kafka/blob/master/src/main/java/org/springframework/integration/kafka/inbound/KafkaMessageSource.java.

The SourcePollingChannelAdapter is a final component to wrap a MessageSource and provide polling functionality.

This way you won't need to rename PubSubInboundChannelAdapter at all.

I'm not sure what is that pubsubMessageChannelMessageConverter, but I believe it is not going to do anything with MessageChannel. Therefore bad name.

On the other hand we are now in the GA phase, so we have to be careful what we are going to break. or just deffer such a work for the next 1.2 (or 2.0) version.

Thanks for understanding!

[elefeint]

Let me see if I understand... with numbered topics for future reference; threading in github is a challenge:

1. For the synchronous pull implementation, I would implement AbstractMessageSource (say, PubSubPullMessageSource), and the client apps then create their own @InboundChannelAdapter with a poller and return an instance of PubSubPullMessageSource?

2. I'd make no changes PubSubInboundChannelAdapter, leaving it as the streaming pull implementation.

3. No deprecations or other changes until 1.1 is GA.

Non-critical-path conversation:
4. My thought with pubsubMessageChannelMessageConverter was to provide automatic conversion for PubSubMessage, per docs :

Alternatively, you can declare a of type MessageConverter with an ID of datatypeChannelMessageConverter, and that converter is used by all channels with a datatype.

[artembilan]

That's not correct to make such a critical change for that datatypeChannelMessageConverter.
Your application might not only have PubSub, but any other integration protocols, so making such a global change is going to be very serious damage for target projects.

Everything else sounds good.
Although I don't think it worth to mention pull in the class name. That is really clear by the MessageSource nature and has to be explained in it Javadocs.