-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
pubsub: allow Ack/Nack outside Receive function #8200
Comments
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
This seems similar to #8170. Would receiving a batch of messages be an appropriate workaround to acking/nacking outside |
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:
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. |
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 I have a few updates to share on that since then. I had originally encouraged setting 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. |
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 Thanks for the tip about 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. |
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! |
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 theReceive()
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 theReceive()
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!
The text was updated successfully, but these errors were encountered: