Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Messenger multiple failed transports#38468

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@monteiro
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#34911
LicenseMIT
Doc PRsymfony/symfony-docs#13489

Strategy applied

  • Pass a map of transports and failed transports to theSendFailedMessageToFailureTransportListener. 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.yamlframework:# no need to set failed transport globally if I want a specific behaviour per transport.failure_transport:failed# all transports have this failed transportmessenger: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:3delay:1000multiplier:2async_no_failure_transport:# it will use the global defined transport if no one is defined.dsn:'%env(MESSENGER_TRANSPORT_DSN)%'retry_strategy:max_retries:3delay:1000multiplier:2async_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:3delay:1000multiplier:2

You can test this feature easily on ademo project. Just follow theREADME.

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

cicoub13, Guite, escees, and 94noni reacted with thumbs up emojimaxhelias, rmikalkenas, cicoub13, and escees reacted with eyes emoji
@monteiro
Copy link
ContributorAuthor

monteiro commentedOct 7, 2020•
edited
Loading

@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.

@nicolas-grekasnicolas-grekas added this to the5.x milestoneOct 12, 2020
@monteiro
Copy link
ContributorAuthor

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

Copy link
Member

@weaverryanweaverryan left a comment

Choose a reason for hiding this comment

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

Getting close...

@monteiro
Copy link
ContributorAuthor

monteiro commentedDec 31, 2020 via email

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

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

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

@rkazaniszyn
Copy link

Fingers crossed for this to be completed and merged :)

Copy link
Member

@weaverryanweaverryan left a comment

Choose a reason for hiding this comment

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

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

@monteiromonteiroforce-pushed themessenger-multiple-failed-transports branch fromb2cc3f6 to303514eCompareMarch 18, 2021 06:51
@monteiro
Copy link
ContributorAuthor

monteiro commentedMar 22, 2021•
edited
Loading

@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-progressError: Class or interface Prophecy\Prophecy\ProphecySubjectInterface does not existError: Process completed with exit code 2.

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

  • Feel free to test! Now it's ready!
devrck reacted with hooray emoji

Copy link
Member

@weaverryanweaverryan left a comment•
edited
Loading

Choose a reason for hiding this comment

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

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_transport1transports:transport2:dsn:doctrine://defaultoptions:queue_name:transport2failure_transport:failure_transport2failure_transport1:dsn:doctrine://defaultoptions:queue_name:failure_transport1failure_transport2:dsn:doctrine://defaultoptions:queue_name:failure_transport2routing:App\Message\ILikeToFail:transport2

TheILikeToFail fail message is routed totransport2. So if it fails, it is sent tofailure_transport2. Great! Now, if I retry that message fromfailure_transport2 and it fails, it is sent tofailure_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 itsown "failure transport" config and determines that the message should go to the "global failure transport", which isfailure_transport1. But I don't think this is the behavior we want: the message should go back tofailure_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.

$io->comment(sprintf('Showing first %d messages.',$max));
}

$io->comment('Run <comment>messenger:failed:show {id} -vv</comment> to see message details.');
Copy link
Member

Choose a reason for hiding this comment

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

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). Thefind() method in the Doctrine connection class incorrectly does NOT include theAND queue_name = ? part.

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

@monteiro
Copy link
ContributorAuthor

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

chalasr added a commit that referenced this pull requestMar 29, 2021
…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 <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| License       | MITThis 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
chalasr
chalasr previously approved these changesMar 29, 2021
@chalasrchalasr self-requested a reviewMarch 29, 2021 22:30
@chalasrchalasr removed their request for reviewMarch 29, 2021 22:31
@chalasrchalasr dismissed theirstale reviewMarch 29, 2021 22:31

Bad button!

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

Great work. Some comments above

@monteiro
Copy link
ContributorAuthor

monteiro commentedMar 30, 2021•
edited
Loading

Thanks@chalasr. Already fixed with your suggestions.

@chalasrchalasrforce-pushed themessenger-multiple-failed-transports branch from605fc7b to5810b6cCompareMarch 30, 2021 14:30
@chalasr
Copy link
Member

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

@chalasrchalasr merged commit7327919 intosymfony:5.xMar 30, 2021
@monteiromonteiro deleted the messenger-multiple-failed-transports branchMarch 30, 2021 14:42
@monteiro
Copy link
ContributorAuthor

Thanks@chalasr again!

chalasr reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestApr 1, 2021
…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#40643Commits-------079cb31 [Messenger] fix deprecation layer in AbstractFailedMessagesCommand
@fabpotfabpot mentioned this pull requestApr 18, 2021
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestApr 20, 2021
This PR was squashed before being merged into the 5.3-dev branch.Discussion----------[Messenger] Multiple failed transports<!--If your pull request fixes a BUG, use the oldest maintained branch that containsthe bug (seehttps://symfony.com/roadmap for the list of maintained branches).If your pull request documents a NEW FEATURE, use the same Symfony branch wherethe feature was introduced (and `master` for features of unreleased versions).-->### New Feature: Messenger: Multiple Failure Transports Transportscloses#15168This PR adds the documentation related to PR:symfony/symfony#38468What 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
nicolas-grekas added a commit that referenced this pull requestJun 8, 2021
…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 <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tickets       |Fix#41545 ... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->| License       | MITWhen 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-symfonyCommits-------aa0e166 [Messenger] fix BC for FrameworkBundle 4.4 with a non-existence alias being used
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@weaverryanweaverryanweaverryan approved these changes

@chalasrchalasrchalasr approved these changes

@srozesrozeAwaiting requested review from sroze

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

[Messenger][Feature] Failed queue per transport

7 participants

@monteiro@escees@rkazaniszyn@chalasr@weaverryan@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp