sroze / messenger-enqueue-transport

Uses Enqueue with Symfony's Messenger component.
MIT License
190 stars 53 forks source link

Fix snsqs redelivery #104

Closed andrewmy closed 2 years ago

andrewmy commented 3 years ago

When resending a message for retry on enqueue/snsqs transport, we need to send the message to the queue rather than the topic.

andrewmy commented 2 years ago

@Nyholm can this be merged?

Nyholm commented 2 years ago

Sorry for all the questions.

Do I understand it correctly that if you are using sroze/messenger-enqueue-transport with SnsqsTransport, then you can only use one queue?

I imagined Snsqs to be like AMQP fanout. Ie, you have multiple queues. They way I see it (Im probably wrong) the queue you are using here will always be the same, It comes from a DSN or similar.

What Im asking is: How are we sure that we republish the message on the correct queue?

andrewmy commented 2 years ago

@Nyholm you're right, it's similar to AMQP fanout — we configure a SNS topic to send messages to and a SQS queue (subscribed to the topic) to receive them. Now maybe I misunderstood something but I thought you could configure only one queue using this messenger enqueue transport lib (coming from DSN indeed, like "enqueue://default?receiveTimeout=20000&queue[name]=some-queue&topic[name]=some-topic"), and it will be the correct one.

If we can configure multiple queues here, how do we do it?

andrewmy commented 2 years ago

@Nyholm sorry for pushing this :) what can I do to help this get further?

Nyholm commented 2 years ago

@Nyholm you're right, it's similar to AMQP fanout — we configure a SNS topic to send messages to and a SQS queue (subscribed to the topic) to receive them. Now maybe I misunderstood something but I thought you could configure only one queue using this messenger enqueue transport lib (coming from DSN indeed, like "enqueue://default?receiveTimeout=20000&queue[name]=some-queue&topic[name]=some-topic"), and it will be the correct one.

But with this configuration causes an issue if we merge this PR, right?

You write to SNS topic Foobar. That will fan out to SQS queue A, B and C.

If consumer for queue B fail, you want to repost the message to queue B. Not to Foobar. With a DSN like in your example, you will always put the message back to the same queue, no matter from where it came from. It is too static, right?

andrewmy commented 2 years ago

If consumer for queue B fail, you want to repost the message to queue B. Not to Foobar. With a DSN like in your example, you will always put the message back to the same queue, no matter from where it came from. It is too static, right?

If I understand correctly:

  1. I only can indicate one topic and one queue per snsqs transport;
  2. If there are multiple snsqs transports, the $destination will have the topic and queue of the relevant transport.

If this is right, the retry message goes back to the queue where it came from. Does it make sense?

Nyholm commented 2 years ago
  1. Correct
  2. Im not sure. It looks like $destination will always have topic and queue information. That will depend on the DSN, not the queue message came from.

HM. Let me just confirm something... You write to SNS topic Foobar. That will fan out to SQS queue A, B and C.

If you now use DSN enqueue://default?queue[name]=queue-z&topic[name]=Foobar. You start a consumer, It will read from queue-z, right?

So if your app should read from SQS A and B you need two different DSN, right? The current behavior is to only write to SNS, but your PR make sure you will write directly to the queue when failing.

My worry was that it does not choose queue automatically. But I (think I) understand now that you always has to hard code the queue to read from.

andrewmy commented 2 years ago

But I (think I) understand now that you always has to hard code the queue to read from.

Exactly, I couldn't make it work properly without providing both topic and queue in the DSN.

andrewmy commented 2 years ago

@Nyholm what can I do to help this get further?

andrewmy commented 2 years ago

Great news, thank you! Can we also get a release?