Skip to content
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

Fix snsqs redelivery #104

Merged
merged 2 commits into from Aug 3, 2021
Merged

Fix snsqs redelivery #104

merged 2 commits into from Aug 3, 2021

Conversation

andrewmy
Copy link
Contributor

@andrewmy andrewmy commented Jul 9, 2021

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

@andrewmy andrewmy requested a review from Nyholm July 11, 2021 10:00
@andrewmy
Copy link
Contributor Author

@Nyholm can this be merged?

@Nyholm
Copy link
Collaborator

Nyholm commented Jul 19, 2021

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@Nyholm
Copy link
Collaborator

Nyholm commented Jul 29, 2021

@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
Copy link
Contributor Author

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
Copy link
Collaborator

Nyholm commented Jul 29, 2021

  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
Copy link
Contributor Author

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
Copy link
Contributor Author

andrewmy commented Aug 3, 2021

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

Copy link
Collaborator

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Sorry for my slow responses and slow mind.

This all look good. Thank you

@Nyholm Nyholm merged commit c8b6cb4 into sroze:master Aug 3, 2021
@andrewmy
Copy link
Contributor Author

andrewmy commented Aug 3, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants