search

dashbitco / broadway_sqs

A Broadway producer for Amazon SQS
92 stars 32 forks source link

Proposal on dynamic visibility timeout #18

Closed albertored closed 5 years ago

albertored commented 5 years ago

I have a legacy application that uses GenStage for processing messages from an SQS queue. The processing takes a variable amount of time so I have implemented some sort of registry that handles the visibility timeout of messages. When a message arrives a timer that periodically extends its visibility is started. When the processing fails the visibility timeout is set to 0 and if instead the processing is successful the message is deleted from the queue. After some configured time the timer is stopped and the visibility set to 0.

I don't know if this is a common pattern and I don't think the whole "visibility registry" logic should be included in this project.

I, however, suggest some small additions to have more flexibility in situations like this.

The first is to have the Acknowledger behavior in a separate module and let it be overridden by a config parameter. Regarding my use case, this would allow a user to perform custom actions on success/fail outcomes by implementing its own ack logic.

The second one is to have an optional callback to be executed after wrap_received_messages to let a user modify the received messages or perform actions. For instance, I will use it for starting the visibility renew timers. Maybe also the :transformer option of producers can be used for this but it is executed for every single message and not for the whole received list and I it is executed on the handle_demand callback, not when actually the messages are received.

josevalim commented 5 years ago

I believe you can achieve what you propose by having a custom broadway_sqs client that wraps our own. So what you do is that you pass your own client and internally it delegates to BroadwaySQS.ExAWS. This will allow you to wrap all of the actions you have mentioned.

albertored commented 5 years ago

Hi @josevalim, thanks for the reply, this is indeed a good idea, thanks!

The only small problem I still see is with the Acknowledger. Functions that build and put the :acknowledger in the message are private so I can't change them and the acknowledger in messages will still point to the original SQS client module. This is not blocking because in my receive_messages function I can map over messages and override the acknowledger module but it seems to me that having it as a configuration parameter would be cleaner.

It is not configurable because in general the acknowledger implementation is tightly bound to the producer one and the user is not supposed to change its logic or there is some other reason?

josevalim commented 5 years ago

It is totally fine for a producer to change the ack fields. So you can wrap it. The reason we don’t have a public API (yet) is because for now we are mostly focused on the user API. --

José Valimwww.plataformatec.com.br http://www.plataformatec.com.br/Founder and Director of R&D

albertored commented 5 years ago

I tried and it works without problems, thanks again

axelson commented 3 years ago

In case anyone is looking for how to do this, Alberto was kind enough to prepare a gist of how he accomplished this: https://gist.github.com/albertored/2fbbf304da765716f88336558698d0c2

Although do note that it is using old versions of broadway (0.3.0) and broadway_sqs (0.2.0) and may not work without modification.