Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Messenger] On failure retry, make message appear received from original sender#31425
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
[Messenger] On failure retry, make message appear received from original sender#31425
Uh oh!
There was an error while loading.Please reload this page.
Conversation
4e6848a todb47a21Compare8ad8b4c toce9154fComparesrc/Symfony/Component/Messenger/Command/AbstractFailedMessagesCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
| }else { | ||
| // get() and ask messages one-by-one | ||
| $count =$this->runWorker($this->getReceiver(),$io,$shouldForce); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why it is only collecting one message at a time a few lines aboveforeach ($receiver->all(1) as $envelope) {? Wouldn't it be better to batch more messages to avoid querying doctrine for each message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmm. We could... but I don't see a lot of benefit. This is the interactive mode, and we're asking the user if they want to retry one-by-one. In theory, something else could also be reading from the failure transport at the same time, so getting whatever the latest id is one-by-one seems sensible.
| $envelope =$event->getEnvelope(); | ||
| // avoid re-sending to the failed sender |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I see that it makes sense. Otherwise consuming the failure transport could be an endless loop of processing and failing and requeuing.
But I think it should be explicitly warned in the retry command that the message will be gone after retrying it even if it fails again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep, exactly. However, the failure transport acts like any normal transport. If the transport has retries configured (by default it does, and the default is 3 tries), the message will be retried 3 times on the failure transport before being discarded. In this PR, I even added some extra info on the retry command that says that this message has already been tried 1/2/3... times on the failure transport previously.
ce9154f toecff07bCompareweaverryan commentedMay 9, 2019
This should be ready - tests passing - ready for review! |
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
0927cf0 to62f0bb6Compareweaverryan commentedMay 10, 2019
Ready to go again. This is mostly a small fix + some refactoring based on@Tobion's recommendations. The vast majority of the added lines are in a robust new integration test. |
62f0bb6 to80b5df2Comparefabpot commentedMay 11, 2019
Thank you@weaverryan. |
… from original sender (weaverryan)This PR was squashed before being merged into the 4.3 branch (closes#31425).Discussion----------[Messenger] On failure retry, make message appear received from original sender| Q | A| ------------- | ---| Branch? | master (4.3)| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#31413| License | MIT| Doc PR |symfony/symfony-docs#11236Fixes a bug when using the transport-based handler config from#30958. This also adds a pretty robust integration test that dispatches a complex message with transport-based handler config, failures, failure transport, etc - and verifies the correct behavior.Cheers!Commits-------80b5df2 [Messenger] On failure retry, make message appear received from original sender
Uh oh!
There was an error while loading.Please reload this page.
Fixes a bug when using the transport-based handler config from#30958. This also adds a pretty robust integration test that dispatches a complex message with transport-based handler config, failures, failure transport, etc - and verifies the correct behavior.
Cheers!