[Messenger] Ability to distinguish retry and delay actions #36864

theravel · 2020-05-18T23:47:26Z

Added ability to distinguish retry and delay actions so that different "x-dead-letter-exchange" exchange name will be used in different scenarios.

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	-
License	MIT
Doc PR

This is a bug which existed since v4.4. The following scenario is possible:

There are two queues: A and B, both are bound to the same routing key via "topic" exchange (two different applications for example).
A message is published to this routing key to "topic" exchange.
Consumer of queue A handles it correctly and acknowledges the message.
Consumer of queue B throws and exception and message goes to retry (for example to queue delay_delays_key_5).
Once message expired in delay_delays_key_5, it is delivered again to both A and B (again consumed by consumer A).

Expected: behavior of consumer B should not cause message duplication to queue A.

It is required to make a change of name of temporary delay queue (otherwise "delay" and "retry" queues have incompatible declaration arguments). I left queue_name_pattern as is to keep settings of connection backward compatible, but changed internals of queue name construction.

nicolas-grekas · 2020-05-19T08:28:27Z

In my implementation I could not avoid minor incompatible change: queue_name_pattern has changed, so looks like this cannot be released under 4.4 or 5.0. Can we include it into 5.1 please?

Neither would this be allowed in master since it would break apps migrating to 5.2. Can we find another idea?

theravel · 2020-05-19T09:06:08Z

Neither would this be allowed in master since it would break apps migrating to 5.2. Can we find another idea?

@nicolas-grekas Thank you for the feedback. I changed it: now queue_name_pattern remains backward compatible, but I changed internals of queue name construction.

Should I change target branch in this case?

nicolas-grekas · 2020-05-19T09:13:27Z

Yes please if this should be considered as a bugfix (I didn't check that part).

theravel · 2020-05-19T22:22:21Z

@nicolas-grekas changed target to 4.4

Tobion · 2020-05-23T20:50:48Z

src/Symfony/Component/Messenger/Transport/AmqpExt/AmqpStamp.php

@@ -45,7 +46,12 @@ public function getAttributes(): array
        return $this->attributes;
    }

-    public static function createFromAmqpEnvelope(\AMQPEnvelope $amqpEnvelope, self $previousStamp = null): self
+    public function isEnvelopeRedelivered(): bool


This is really confusing. AMQP also has the concept of redelivery. But this is not the same as the redelivery in the messenger component. Since this is the stamp to encode the amqp flags, this is really unexpected. So this needs a different name or not be exposed at all.

This has been renamed

Tobion · 2020-05-23T21:01:14Z

#36196 and #36212 already try to solve the same problem.

s-code · 2020-05-28T14:17:04Z

@theravel i was able to solve this and few other problems without changing Messenger connection class, but it's little bit tricky. Here is an example https://github.com/s-code/amqp-extra-bundle. Take a look on ExtraAmqpFactory, ExtraAmqpSender, ExtraAmqpQueue, ExtraAmqpExchange.
As for me, splitting of retry and delay logic is a must have for amqp transport.

weaverryan · 2020-10-22T20:14:54Z

On a high level, this makes sense to me:

If a message is in queue A, and I read it from queue A and it fails, then it should go back onto queue A only.

Messenger handles retries by actually ack'ing the original message and redelivering it. But ultimately, the desired behavior (I think) would be what I described above and what this PR does.

Though, apparently this and #38486 solve the same problem (is it the exact same problem?) but in different ways. @s-code could you comment on what your PR does versus this one?

Thanks :)

s-code · 2020-10-22T21:18:22Z

@weaverryan you are right. This two pull requests solve the same problem. The difference is in next things:

Delay queue naming convention.

[Messenger] Ability to distinguish retry and delay actions #36864 requires its change from delay_{exchange}_{routing_key}_{time} to delay_{exchange}_{routing_key}_{time}_(retry|delay). So as result we have delay_messages_message1_500_retry, delay_messages_message1_500_delay queues names for default configuration
In [Messenger] Add possibility to route delayed messages to the origin queue directly #38486 there is no changes in this convention. In case of retry {exchange} = default exchange = '', {routing_key} = origin queue name, because for default exchange queue name is using as routing key. So queues names look like delay__queue1_500 and delay_messages_message1_500

[Messenger] Add possibility to route delayed messages to the origin queue directly #38486 allows to use default exchange for delay and retry logic. If in configuration we define delay.exchange_name as '', no extra ("delays") exchange will be created. For me it's more elegant and clean solution

nicolas-grekas · 2021-04-12T08:59:52Z

(please add a changelog entry in the bridge)

theravel · 2021-04-12T11:41:17Z

@nicolas-grekas Changelog entry added

src/Symfony/Component/Messenger/Bridge/Amqp/CHANGELOG.md

src/Symfony/Component/Messenger/Bridge/Amqp/Transport/AmqpStamp.php

outdated

Nyholm

Im happy with this. Just one question

Nyholm · 2021-04-21T12:51:00Z

src/Symfony/Component/Messenger/Bridge/Amqp/Transport/AmqpStamp.php

@@ -45,7 +46,7 @@ public function getAttributes(): array
        return $this->attributes;
    }

-    public static function createFromAmqpEnvelope(\AMQPEnvelope $amqpEnvelope, self $previousStamp = null): self
+    public static function createFromAmqpEnvelope(\AMQPEnvelope $amqpEnvelope, self $previousStamp = null, string $retryRoutingKey = null): self


We are using $amqpReceivedStamp->getQueueName() to populate the $retryRoutingKey, is that really correct?

A routing key is not the same as the queue name.

(It is probably correct, but I just need you to clarify.)

This is correct. We want to distinguish:

delay scenario, where message should be sent to original exchange to original routing key (so that all consumers can get it)

retry scenario, where message should be sent to default (direct) exchange to routing key which matches queue name (in this case it will be received in one queue only)

Are we sure the routing key always matches the queue name?

For direct exchange in AMQP 0-9-1 yes:

The default exchange is a direct exchange with no name (empty string) pre-declared by the broker. It has one special property that makes it very useful for simple applications: every queue that is created is automatically bound to it with a routing key which is the same as the queue name.

See https://www.rabbitmq.com/tutorials/amqp-concepts.html

@Nyholm can you review please?

MichaelKubovic · 2021-04-21T14:16:45Z

I believe it is always the case with direct exchange.

The default exchange is a direct exchange with no name (empty string) pre-declared by the broker. It has one special property that makes it very useful for simple applications: every queue that is created is automatically bound to it with a routing key which is the same as the queue name.

https://www.rabbitmq.com/tutorials/amqp-concepts.html

…

On 21 Apr 2021, at 15:40, Tobias Nyholm ***@***.***> wrote: @Nyholm commented on this pull request. In src/Symfony/Component/Messenger/Bridge/Amqp/Transport/AmqpStamp.php: > @@ -45,7 +46,7 @@ public function getAttributes(): array return $this->attributes; } - public static function createFromAmqpEnvelope(\AMQPEnvelope $amqpEnvelope, self $previousStamp = null): self + public static function createFromAmqpEnvelope(\AMQPEnvelope $amqpEnvelope, self $previousStamp = null, string $retryRoutingKey = null): self Are we sure the routing key always matches the queue name? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

Nyholm · 2021-05-10T20:42:46Z

Thank you @theravel and @MichaelKubovic for confirming.

Nyholm · 2021-05-10T20:46:39Z

Thank your for this feature. Please update the docs too =)

theravel requested a review from sroze as a code owner May 18, 2020 23:47

carsonbot added Status: Needs Review Bug labels May 18, 2020

theravel force-pushed the fix-retry-broadcasting branch from 102bf8e to f6e8c45 Compare May 18, 2020 23:49

theravel changed the title ~~Ability to distinguish retry and delay actions~~ [Messenger] Ability to distinguish retry and delay actions May 18, 2020

nicolas-grekas added this to the 4.4 milestone May 19, 2020

theravel force-pushed the fix-retry-broadcasting branch from 798e402 to d3124bf Compare May 19, 2020 09:01

theravel force-pushed the fix-retry-broadcasting branch from d3124bf to aa81532 Compare May 19, 2020 22:18

theravel changed the base branch from master to 4.4 May 19, 2020 22:19

Tobion previously requested changes May 23, 2020

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 23, 2020

fabpot added the Messenger label Aug 17, 2020

fabpot mentioned this pull request Sep 6, 2020

[Messenger] Add possibility to route delayed messages to the origin queue directly #36212

Closed

theravel force-pushed the fix-retry-broadcasting branch from aa81532 to 5d084a2 Compare March 28, 2021 20:49

theravel requested review from chalasr, dunglas, jderusse, lyrixx, wouterj, xabbuh and yceruto as code owners March 28, 2021 20:49

theravel requested a review from nicolas-grekas April 11, 2021 22:43

nicolas-grekas approved these changes Apr 12, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 12, 2021

nicolas-grekas reviewed Apr 12, 2021

View reviewed changes

src/Symfony/Component/Messenger/Bridge/Amqp/CHANGELOG.md Outdated Show resolved Hide resolved

nicolas-grekas reviewed Apr 12, 2021

View reviewed changes

src/Symfony/Component/Messenger/Bridge/Amqp/CHANGELOG.md Outdated Show resolved Hide resolved

nicolas-grekas added the Ready label Apr 13, 2021

Nyholm added Feature and removed Bug labels Apr 19, 2021

Nyholm reviewed Apr 19, 2021

View reviewed changes

src/Symfony/Component/Messenger/Bridge/Amqp/Transport/AmqpStamp.php Outdated Show resolved Hide resolved

Nyholm modified the milestones: 5.x, 5.3 Apr 19, 2021

theravel requested a review from Nyholm April 20, 2021 07:51

carsonbot added Status: Needs Review and removed Status: Reviewed labels Apr 20, 2021

Nyholm reviewed Apr 21, 2021

View reviewed changes

theravel requested a review from Nyholm April 21, 2021 13:39

goetas mentioned this pull request May 2, 2021

[Messenger] Add possibility to route delayed messages to the origin queue directly #38486

Closed

[Messenger] Ability to distinguish retry and delay actions

417aaab

Nyholm force-pushed the fix-retry-broadcasting branch from 35a6ada to 417aaab Compare May 10, 2021 20:46

Nyholm mentioned this pull request May 10, 2021

[Messenger] Ability to distinguish retry and delay actions symfony/symfony-docs#15327

Closed

Nyholm merged commit 649d115 into symfony:5.x May 10, 2021

fabpot mentioned this pull request May 12, 2021

Release v5.3.0-BETA4 #41196

Merged

goetas mentioned this pull request Sep 22, 2021

[Messenger] Retry strategy causes potentially exponential message duplication when more than one queue is connected to the same exchange #41007

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Ability to distinguish retry and delay actions #36864

[Messenger] Ability to distinguish retry and delay actions #36864

theravel commented May 18, 2020 •

edited by nicolas-grekas

Loading

nicolas-grekas commented May 19, 2020

theravel commented May 19, 2020 •

edited

Loading

nicolas-grekas commented May 19, 2020

theravel commented May 19, 2020

Tobion May 23, 2020

theravel Apr 6, 2021

Tobion commented May 23, 2020

s-code commented May 28, 2020 •

edited

Loading

weaverryan commented Oct 22, 2020

s-code commented Oct 22, 2020 •

edited

Loading

nicolas-grekas commented Apr 12, 2021

theravel commented Apr 12, 2021

Nyholm left a comment

Nyholm Apr 21, 2021

theravel Apr 21, 2021

Nyholm Apr 21, 2021

theravel Apr 21, 2021

theravel Apr 28, 2021

MichaelKubovic commented Apr 21, 2021 via email

Nyholm commented May 10, 2021

Nyholm commented May 10, 2021

[Messenger] Ability to distinguish retry and delay actions #36864

[Messenger] Ability to distinguish retry and delay actions #36864

Conversation

theravel commented May 18, 2020 • edited by nicolas-grekas Loading

nicolas-grekas commented May 19, 2020

theravel commented May 19, 2020 • edited Loading

nicolas-grekas commented May 19, 2020

theravel commented May 19, 2020

Tobion May 23, 2020

Choose a reason for hiding this comment

theravel Apr 6, 2021

Choose a reason for hiding this comment

Tobion commented May 23, 2020

s-code commented May 28, 2020 • edited Loading

weaverryan commented Oct 22, 2020

s-code commented Oct 22, 2020 • edited Loading

nicolas-grekas commented Apr 12, 2021

theravel commented Apr 12, 2021

Nyholm left a comment

Choose a reason for hiding this comment

Nyholm Apr 21, 2021

Choose a reason for hiding this comment

theravel Apr 21, 2021

Choose a reason for hiding this comment

Nyholm Apr 21, 2021

Choose a reason for hiding this comment

theravel Apr 21, 2021

Choose a reason for hiding this comment

theravel Apr 28, 2021

Choose a reason for hiding this comment

MichaelKubovic commented Apr 21, 2021 via email

Nyholm commented May 10, 2021

Nyholm commented May 10, 2021

theravel commented May 18, 2020 •

edited by nicolas-grekas

Loading

theravel commented May 19, 2020 •

edited

Loading

s-code commented May 28, 2020 •

edited

Loading

s-code commented Oct 22, 2020 •

edited

Loading