pubsub: allow Ack/Nack outside Receive function

[horgh]

Is your feature request related to a problem? Please describe. I noticed that the docs were changed (in #5718) to say that Ack()/Nack() must be called inside the Receive() callback. Unfortunately, I have several applications built around the pubsub package that predate this and currently violate it.

Describe the solution you'd like I was wondering if it would be possible to support the pattern of calling Ack()/Nack() outside of the Receive() callback. The reason is we want to handle messages in batches. Not all of our pubsub based applications benefit from working this way, but several do.

Describe alternatives you've considered I haven't tried to reimplement these applications yet to meet the changed requirement, but that's what I'll be trying, if the pattern can't be supported!

[hongalex]

Hey, thanks for filing an issue. I think you know this already but just to clarify for others, the behavior of the library hasn't changed, and it was only the documentation that was updated.

Since you seem to have been using your application for a while and calling ack outside of Receive, I wanted to check to see if those applications have been behaving poorly. To reiterate, calling ack/nack outside of Receive isn't recommended since flow control isn't guaranteed (you might receive more than MaxOutstandingMessages at once) and during shutdown, graceful shutdown doesn't happen. That is, messages that are in progress outside of the callback when the context is cancelled will stop being processed immediately. If these are acceptable tradeoffs for you, I would continue to use your application as is.

handle messages in batches

This seems similar to #8170. Would receiving a batch of messages be an appropriate workaround to acking/nacking outside Receive?

[horgh]

Thanks for the reply!

The apps have generally been performing fine. We noticed the docs changed when working on a different issue. There have been occasional issues with these apps, but I'm not sure if they could be related to this:

• Occasionally they get backed up and don't process the messages as quickly as usual. However I believe this is probably due to slowness with what we do with the messages.
• In the past, we had issues where messages started getting delivered to our workers much more slowly (I had #4073 about this). We ended up working around this by restarting the apps whenever we detect it, which we still have in place (which I'm not sure is still necessary or not).

It sounds like these would probably not be caused by not following the required Ack/Nack pattern though. I'm inclined to update things so we could rule it out though.

Regarding receiving batches of messages as described in #8170 - yes, I think that would help. We are batching for similar reasons as described in that issue: We want to insert/update many rows in a database at once, each of which is one Pub/Sub message. What we do right now is send messages to a channel in Receive(), then the consumer of the channel receives messages up to a certain limit or until a deadline is hit before processing the batch.

[hongalex]

Thanks for the context. I just reviewed the original issue since it's been a while. I think we had chalked it up to server issues before, though needing to restart frequently isn't ideal from a user perspective, especially given the issue of graceful shutdowns not working when acking outside Receive's callback.

I have a few updates to share on that since then. I had originally encouraged setting NumGoroutines higher to try and get more throughput, but since then we've been encouraging users to lower this number when possible. These goroutines correspond to the number of gRPC streams opened and each stream can handle 10 MB/s of messages. If you're still publishing at 1k msg/sec, and each message is less than 10kb on average, even just 1 stream can be sufficient. We found that setting the number of streams too high is redundant at best and harmful at worse (since each stream adds additional overhead).

I think the message backlog building up has to do with messages expiring since they aren't being handled fast enough before they are expired. This should be looked at from a memory standpoint as well to make sure cascading messages aren't causing memory blowup.

[horgh]

Yes, that old problem went away, so server issues seems likely. I don't think we restart very frequently currently, so I suspect it's benign, even if pointless. We restart only when Receive() returns (rather than trying to re-Receive()).

Thanks for the tip about NumGoroutines! I still have the app set at 50 from that issue. I'll get that reduced.

It sounds like those issues probably would not be caused by this Ack/Nack pattern though. And it also sounds like it is unlikely to be negatively affecting us. I'm not aware of any ongoing issues with the apps, but I get worried when I have what amounts to misuse of an API :-P.

I suspect the answer to whether the pattern will be supported without those caveats generally is no though :-). I think we'll still want to rework our apps just in case, unless you think that batching capability will be coming any time soon.

Thank you for your help & for your work on the package! Pub/Sub is pretty great.

[hongalex]

batching capability will be coming any time soon

Currently, there are higher priority items outstanding (like shipping OpenTelemetry tracing and supporting a couple new export subscriptions features) before we start delving into implementing features like this. However, it's definitely in our radar!