pubsub: allow message to modify ack_deadline

[josedonizetti]

The ruby lib has a very useful feature, that I'd like to see on the golang lib. A received message can modify their own ack_deadline: https://github.com/GoogleCloudPlatform/google-cloud-ruby/blob/master/google-cloud-pubsub/lib/google/cloud/pubsub/received_message.rb#L159

The go version only allow a call for Ack/Nack: https://github.com/GoogleCloudPlatform/google-cloud-go/blob/master/pubsub/message.go

Does it make sense? This is something I'm willing to send a PR if you agree.

[jeanbza]

The Go pubsub client library does ack deadline extension for you automatically. We haven't updated the documentation to reflect this yet, but you can read some of the rough draft here: https://code-review.googlesource.com/c/gocloud/+/32914

Does this suffice for your usecase, or do you need finer grained control? If so, could you describe your usecase and why this knob might be useful?

[josedonizetti]

The usecase is to defer processing a message due to some external factor. For example, I received a message, but I can't process it now, I can't Ack it or else it is marked as done, if I Nack it, the message will be resend almost immediately, but I still can't process it. What I want to do, is to notify the pubsub that this message is still unacked, and I only want to receive it back a few seconds from now (ack_deadline). Make sense?

[jeanbza]

Ah, right on. For that case I believe you could hold the message locally (all the while the library is extending its ack deadline) and then Nack it after the appropriate amount of time. If it's held less than 10 minutes, it should not expire.

Alternatively and less ideally, you could use the lower-level Pull API (an example here).

Would either of these work for you?

[josedonizetti]

Don't you think that holding the message until the time consumes space on the Receive flowController, and if one does it to multiple msgs there will be a good amount of locked messages, not letting the msgs flow, with a Delay func the lock release could be done, clearing the flowController, like Ack/Nack.

[jeanbza]

It depends on your usecase. Is this a once-off kind of thing? Then this would probably work. If you intend on repeating this behavior with frequency, you're right, it would lock your flow controller. It sounds like it's the latter case, so I'll mark this as a feature request.

[jba]

Modifying the ack deadline wouldn't be enough. You must either ack or nack to release the message's slot in the flow controller.

Instead, I'd recommend increasing the flow control settings so you can accommodate holding on to the extra messages.

We've had requests for this feature before, but PubSub doesn't offer it. See #691 for more information.

[josedonizetti]

@jba But adding a Delay func to Message will basically work like Nack, but instead of doing a sendMoAck with 0 it will have a different ack_deadline passed as argument, and it will also release the flowController.

https://github.com/GoogleCloudPlatform/google-cloud-go/blob/933c4eb7eaa5ce52e869c8448e857044bab33b59/pubsub/iterator.go#L308

https://github.com/GoogleCloudPlatform/google-cloud-go/blob/933c4eb7eaa5ce52e869c8448e857044bab33b59/pubsub/iterator.go#L358

[jba]

Whatever ModAck you send will be overridden by the ModAck done by our keepalive. Unless Delay also removes the message from our keepalive list.

[josedonizetti]

@jba Is this something that worth adding to the lib? Or the team recommendation is to use the lowlevel api? If you think it worth I can give a try and send a PR, if not we can close this.

[jba]

I think it makes sense. I like the name "Forget" better than "Delay". In addition to releasing the flow controller and calling ModAck, it should also remove the message from the keepalive map.

Note that the naive implementation will be quite inefficient, generating an RPC for each message. (Normally we batch messages in RPCs to avoid this.) That's a serious concern, because we want and expect this client to be used at scale.

We could just not do the RPC call. The message would then expire whenever its current deadline was up, whenever that was (a function of time-to-ack). That would be much simpler, at the cost of having no control over the redelivery time.

Or, we could have a single configurable value for all Forget calls. Or maybe round the delay up to the nearest second?

Let's figure out the design before you start implementing. And by the way, we use gerrit, not github, for our PRs: see CONTRIBUTING.md.

[jba]

I just looked at the ruby implemention linked above. They treat delay as a synonym for a call to modifyAckDeadline. I don't know how they deal with avoiding keepalive; I didn't see that code. But they definitely make one RPC per call of delay. That's not the right design for Go.

[josedonizetti]

@jba Considering using the lib at scale, and also giving the user some control over redelivery time, having a global Forget value seems best path here. Rounding to the nearest second brings more complexity to the already complex Iterator code. WDYT?

[jba]

Maybe not global, but per-Subscription.

[josedonizetti]

True, any other consideration? Gonna start to write something around it to gather more feedback.

[jba]

We should still batch the calls. You can make the call the same time we do nacks (the nackTicker case in the sender method).

[jba]

Apologies: we are not going to add this feature to the Go client library, despite the fact that Ruby has it. See #691 (comment) for the rationale.