Messenger multiple failed transports #38468

monteiro · 2020-10-07T16:47:39Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #34911
License	MIT
Doc PR	symfony/symfony-docs#13489

Strategy applied

Pass a map of transports and failed transports to the SendFailedMessageToFailureTransportListener. This way we re-use the same listener.
Local failed transport has more priority than a global failed transport defined.

Configuration example

# config/packages/messenger.yaml
framework:
    # no need to set failed transport globally if I want a specific behaviour per transport.
    failure_transport: failed # all transports have this failed transport
    messenger:
        transports:
            failed: 'doctrine://default?queue_name=failed'
            failed_important: 'doctrine://default?queue_name=failed_important'
            async:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                failure_transport: failed # takes precedence over the global defined "failed_transport"
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2
            async_no_failure_transport: # it will use the global defined transport if no one is defined.
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2
            async_send_specific_failure_queue:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                failed_transport: failed_important # takes precedence over the global defined "failed_transport"
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2

You can test this feature easily on a demo project. Just follow the README.

More information on issue #34911

What needs to be done so this can be merged:

validate strategy
update src/**/CHANGELOG.md files
update tests to cover all cases
create doc PR

monteiro · 2020-10-07T16:59:30Z

@weaverryan i have removed some of the tests because of merging conflicts.. since this is still being changed a lot... after we agree on an implementation it’s easy to fix them.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Bundle/FrameworkBundle/Resources/config/console.php

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

monteiro · 2020-10-15T18:27:20Z

@weaverryan I just uploaded a working version of your proposal. Feel free to review, before I do the tests. Thanks!

weaverryan

Getting close...

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

src/Symfony/Bundle/FrameworkBundle/Resources/config/console.php

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

monteiro · 2020-12-31T12:35:44Z

Waiting for Ryan to help me. He has been reviewing my PR since the beginning. Let me talk with him, to see what can we do. Thanks for pinging.

…

On Thu, Dec 31, 2020 at 9:04 AM Kamil Ścisłowski ***@***.***> wrote: How long it will take to be merged into master? @monteiro <https://github.com/monteiro> are you still working on it? :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38468 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAASFW36LHEU3O7E3DDWKWTSXQ5ANANCNFSM4SHURUVA> .

escees · 2020-12-31T12:43:39Z

Sure, I've deleted previous comment because I wasn't sure if it's appropriate place to ask :) But I appreciate for answering. For me and I see that not only for me it's really important feature that is missing from core.

monteiro · 2021-01-01T13:54:41Z

@escees no worries. I already talked with @weaverryan to help me during this 2021 to merge this missing feature.

rkazaniszyn · 2021-03-12T11:16:59Z

Fingers crossed for this to be completed and merged :)

weaverryan

Thanks for you work on this - we're getting ever close - so many little details :)

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php

src/Symfony/Component/Messenger/EventListener/SendFailedMessageToFailureTransportListener.php

src/Symfony/Component/Messenger/Command/FailedMessagesRetryCommand.php

src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php

monteiro · 2021-03-22T11:43:34Z

@weaverryan I have done all changes, including fixing all the tests:

Most code is from the "legacy" tests. I have added the annotation so we support both ways until the switch (moving from SenderInterface to a ServiceLocator) for the major version.
There is a psalm error:

Run ./vendor/bin/psalm.phar --output-format=github --no-progress
Error: Class or interface Prophecy\Prophecy\ProphecySubjectInterface does not exist
Error: Process completed with exit code 2.

which I don't know how to fix, related to that weird extra parameter code in https://github.com/symfony/symfony/pull/38468/files#diff-9213ec846d94497cc34a4b922373cb52e0d53015604427a4aec0661289c58ac8R177

Feel free to test! Now it's ready!

weaverryan

EDIT

The below issue is NOT an issue. It's caused by the retry commands being allowed to query the wrong transport. Basically, the fix in #40588 closes this possibility. There is no problem here.

~~I've been playing with this, and it's looking really good - it's close.~~

~~But I just hit one major problem. Suppose this setup"~~

framework:
    messenger:
        failure_transport: failure_transport1

        transports:
            transport2:
                dsn: doctrine://default
                options:
                    queue_name: transport2
                failure_transport: failure_transport2

            failure_transport1:
                dsn: doctrine://default
                options:
                    queue_name: failure_transport1
            failure_transport2:
                dsn: doctrine://default
                options:
                    queue_name: failure_transport2

        routing:
            App\Message\ILikeToFail: transport2

The ILikeToFail fail message is routed to transport2. So if it fails, it is sent to failure_transport2. Great! Now, if I retry that message from failure_transport2 and it fails, it is sent to failure_transport1. That is not correct.

It makes sense why it's doing this. failure_transport2 is a normal transport. So when the message fails, it actually looks at its own "failure transport" config and determines that the message should go to the "global failure transport", which is failure_transport1. But I don't think this is the behavior we want: the message should go back to failure_transport2 after failing. That is THIS message's failure transport, and it should remain here until it is successful or removed because it hits the retry limit.

tl;dr We need a way to respect the original failure transport that message was routed to. We probably need (if we don't have this already) a stamp to save the "failure transport name" onto a message, so it can be read when being re-delivered after the messages fails a retry from the failure queue.

src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php

src/Symfony/Component/Messenger/Command/FailedMessagesRetryCommand.php

weaverryan · 2021-03-23T14:23:50Z

src/Symfony/Component/Messenger/Command/FailedMessagesShowCommand.php

@@ -122,10 +129,10 @@ private function listMessages(SymfonyStyle $io, int $max)
        $io->comment('Run <comment>messenger:failed:show {id} -vv</comment> to see message details.');


this line (and a few other ones in this command... and maybe other commands, like:retry and :remove) now needs to include --failure-transport= if that option was passed. Otherwise, you will be looking for the message in the wrong transport.

Btw, with Doctrine as your failure transport, forgetting the --failure-transport= option actually DOES work... but due to a bug I just discovered (that we should fix, in a different PR). The find() method in the Doctrine connection class incorrectly does NOT include the AND queue_name = ? part.

symfony/src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/Connection.php

Lines 278 to 281 in 2da27bb

public function find($id): ?array

{

$queryBuilder = $this->createQueryBuilder()

->where('m.id = ?');

src/Symfony/Component/Messenger/Command/FailedMessagesRetryCommand.php

src/Symfony/Component/Messenger/Command/FailedMessagesShowCommand.php

monteiro · 2021-03-23T17:50:35Z

Pending to fix the console command tests after adding the list of transports and the warning.

@weaverryan

…ransport (monteiro) This PR was merged into the 4.4 branch. Discussion ---------- add missing queue_name to find(id) in doctrine messenger transport | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no  | Deprecations? | no  | License | MIT This bug was noticed by @weaverryan when reviewing the PR: #38468 related to multiple failure transports. Commits ------- bb26a92 add missing queue_name to find(id) in doctrine messenger transport

src/Symfony/Component/Messenger/EventListener/SendFailedMessageToFailureTransportListener.php

src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php

Bad button!

chalasr

Great work. Some comments above

monteiro · 2021-03-30T14:14:05Z

Thanks @chalasr. Already fixed with your suggestions.

chalasr · 2021-03-30T14:30:22Z

It took time, but here we go, this is in now. Thank you very much @monteiro.

monteiro · 2021-03-30T14:42:20Z

Thanks @chalasr again!

…sCommand (nicolas-grekas) This PR was merged into the 5.3-dev branch. Discussion ---------- [Messenger] fix deprecation layer in AbstractFailedMessagesCommand | Q | A | ------------- | --- | Branch? | 5.x | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Added in #38468, spotted in #40643 Commits ------- 079cb31 [Messenger] fix deprecation layer in AbstractFailedMessagesCommand

This PR was squashed before being merged into the 5.3-dev branch. Discussion ---------- [Messenger] Multiple failed transports  ### New Feature: Messenger: Multiple Failure Transports Transports closes #15168 This PR adds the documentation related to PR: symfony/symfony#38468 What it needs to be clear in the documentation: - You can define multiple transports, one per transport (instead of the global one, as it is supported today) - If you don't define a global failure transport and if the transport does not have the `failure_transport` configured the messages will be discarded - If you define both a global and at the transport level, the `failure_transport` configuration, the one taken into account is at the transport level configuration. - The failed commands have an option argument to specify the transport level `--transport`. Without arguments is the global failed transport taken into account. Commits ------- d784895 [Messenger] Multiple failed transports

…tence alias being used (monteiro) This PR was squashed before being merged into the 5.3 branch. Discussion ---------- [Messenger] fix BC for FrameworkBundle 4.4 with a non-existence alias being used | Q | A | ------------- | --- | Branch? | 5.3 | Bug fix? | yes | New feature? | no  | Deprecations? | no  | Tickets | Fix #41545 ...  | License | MIT When trying to update symfony/messenger to 5.3 while still using symfony/framework-bundle 4.4 there is an error: The service "console.command.messenger_failed_messages_retry" has a dependency on a non-existent service "messenger.failure_transports.default". This is because in Symfony Messenger 5.3, on the PR #38468 to support multiple failure transports we have created an alias, that is only available on the FrameworkBundle 5.3. This fix will use an already existence alias. Sample project to test this behavior: https://github.com/monteiro/PR-41553-symfony Commits ------- aa0e166 [Messenger] fix BC for FrameworkBundle 4.4 with a non-existence alias being used

monteiro requested a review from sroze as a code owner October 7, 2020 16:47

carsonbot added Status: Needs Review Feature labels Oct 7, 2020

monteiro mentioned this pull request Oct 7, 2020

[Messenger] multiple failure transports support #34979

Closed

4 tasks

monteiro force-pushed the messenger-multiple-failed-transports branch from c9bd097 to bcb3783 Compare October 7, 2020 16:58

weaverryan reviewed Oct 8, 2020

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated Show resolved Hide resolved

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated Show resolved Hide resolved

monteiro requested a review from weaverryan October 11, 2020 08:32

weaverryan reviewed Oct 11, 2020

View reviewed changes

nicolas-grekas added this to the 5.x milestone Oct 12, 2020

monteiro requested a review from weaverryan October 15, 2020 18:26

weaverryan reviewed Oct 15, 2020

View reviewed changes

monteiro requested a review from weaverryan October 16, 2020 16:19

weaverryan reviewed Oct 16, 2020

View reviewed changes

weaverryan reviewed Mar 15, 2021

View reviewed changes

monteiro force-pushed the messenger-multiple-failed-transports branch from b2cc3f6 to 303514e Compare March 18, 2021 06:51

monteiro requested a review from weaverryan March 19, 2021 16:18

weaverryan reviewed Mar 19, 2021

View reviewed changes

src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php Outdated Show resolved Hide resolved

weaverryan reviewed Mar 19, 2021

View reviewed changes

src/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php Outdated Show resolved Hide resolved

monteiro requested a review from weaverryan March 22, 2021 11:40

weaverryan reviewed Mar 23, 2021

View reviewed changes

monteiro force-pushed the messenger-multiple-failed-transports branch from c14fa3d to 336c9b6 Compare March 25, 2021 17:38

monteiro mentioned this pull request Mar 25, 2021

Missing queue_name parameter in Doctrine Transport #40588

Closed

chalasr previously approved these changes Mar 29, 2021

View reviewed changes

chalasr self-requested a review March 29, 2021 22:30

carsonbot added Status: Needs Review and removed Status: Reviewed labels Mar 29, 2021

chalasr removed their request for review March 29, 2021 22:31

chalasr requested changes Mar 29, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Mar 29, 2021

chalasr approved these changes Mar 30, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Mar 30, 2021

Messenger multiple failed transports

5810b6c

chalasr force-pushed the messenger-multiple-failed-transports branch from 605fc7b to 5810b6c Compare March 30, 2021 14:30

chalasr merged commit 7327919 into symfony:5.x Mar 30, 2021

monteiro deleted the messenger-multiple-failed-transports branch March 30, 2021 14:42

monteiro mentioned this pull request Mar 30, 2021

[Messenger] Failure transports documentation symfony/symfony-docs#15168

Closed

This was referenced Apr 1, 2021

Use symfony/deprecation-contracts instead of trigger_error #40643

Merged

[Messenger] fix deprecation layer in AbstractFailedMessagesCommand #40667

Merged

monteiro mentioned this pull request Apr 1, 2021

[Messenger] Multiple failed transports symfony/symfony-docs#15177

Merged

fabpot mentioned this pull request Apr 18, 2021

Release v5.3.0-BETA1 #40853

Merged

jordisala1991 mentioned this pull request Jun 4, 2021

Set minimum required version for framework-bundle to 5.3 #41545

Closed

monteiro mentioned this pull request Jun 5, 2021

[Messenger] fix BC for FrameworkBundle 4.4 with a non-existence alias being used #41553

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

Messenger multiple failed transports #38468

Messenger multiple failed transports #38468

monteiro commented Oct 7, 2020

monteiro commented Oct 7, 2020 •

edited

Loading

monteiro commented Oct 15, 2020

weaverryan left a comment

monteiro commented Dec 31, 2020 via email

escees commented Dec 31, 2020

monteiro commented Jan 1, 2021

rkazaniszyn commented Mar 12, 2021

weaverryan left a comment

monteiro commented Mar 22, 2021 •

edited

Loading

weaverryan left a comment •

edited

Loading

weaverryan Mar 23, 2021

monteiro commented Mar 23, 2021

chalasr left a comment

monteiro commented Mar 30, 2021 •

edited

Loading

chalasr commented Mar 30, 2021

monteiro commented Mar 30, 2021

		@@ -122,10 +129,10 @@ private function listMessages(SymfonyStyle $io, int $max)
		$io->comment('Run <comment>messenger:failed:show {id} -vv</comment> to see message details.');

	public function find($id): ?array
	{
	$queryBuilder = $this->createQueryBuilder()
	->where('m.id = ?');

Messenger multiple failed transports #38468

Messenger multiple failed transports #38468

Conversation

monteiro commented Oct 7, 2020

Strategy applied

Configuration example

What needs to be done so this can be merged:

monteiro commented Oct 7, 2020 • edited Loading

monteiro commented Oct 15, 2020

weaverryan left a comment

Choose a reason for hiding this comment

monteiro commented Dec 31, 2020 via email

escees commented Dec 31, 2020

monteiro commented Jan 1, 2021

rkazaniszyn commented Mar 12, 2021

weaverryan left a comment

Choose a reason for hiding this comment

monteiro commented Mar 22, 2021 • edited Loading

weaverryan left a comment • edited Loading

Choose a reason for hiding this comment

weaverryan Mar 23, 2021

Choose a reason for hiding this comment

monteiro commented Mar 23, 2021

chalasr left a comment

Choose a reason for hiding this comment

monteiro commented Mar 30, 2021 • edited Loading

chalasr commented Mar 30, 2021

monteiro commented Mar 30, 2021

monteiro commented Oct 7, 2020 •

edited

Loading

monteiro commented Mar 22, 2021 •

edited

Loading

weaverryan left a comment •

edited

Loading

monteiro commented Mar 30, 2021 •

edited

Loading