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] Allows to register handlers on a specific transport#30958
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Koc commentedApr 7, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I see some drawbacks with current configuration:
Please consider move out queue name from dsn and extend routing configuration: framework:messenger:transports:events:amqp://guest:guest@127.0.0.1/%2f/eventsrouting:App\Message\RegisterBet: -transport:eventsqueue:events_projectionhandler:App\MessageHandler\RegisterBetHandler -transport:eventsqueue:events_3rdpartyhandler:App\MessageHandler\SomethingOnBetRegister Handler can be omited for cases when we just pulishing messages and don't comsuming they through Messenger. This kind of config is super easy to understand. We have configuration of message-transport/queue-handler in single place. Of course |
106be1d to1de1cb2Comparesroze commentedApr 7, 2019
@Koc I don't see your point as being against this proposed implementation here. The main point is to create this In the example given, the handler indeed knows about the transport (it's already knowing aboutthings like the Regarding the overhead in terms of configuring the exchange and the queue bindings, you could also use the |
1de1cb2 toaf34fbbComparesroze commentedApr 7, 2019
(requires CHANGELOG when a few 👍 will be around 😉 ) |
sroze commentedApr 7, 2019
Open question: should we only have |
weaverryan commentedApr 7, 2019
Makes sense to me - we do this with the bus name stamp and sender name for retry. |
nicolas-grekas left a comment
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.
LGTM, just a few minor things + rebase needed
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
af34fbb to83758b6Comparesroze commentedApr 22, 2019
@nicolas-grekas@weaverryan updated 👍 |
83758b6 tocc6f13dCompare
weaverryan left a comment
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.
This looks good to me - and I tested it locally.
There's just one piece that bothers me:
publicstaticfunctiongetHandledMessages():iterable {yield RegisterBet::class => ['transport' =>'events_projection', ]; }
Themeaning oftransport is this:
This handler will only be called when the message is received from the
events_projectiontransport
But it's just as reasonable to think it means this:
This handler will be sent to the
events_projectiontransport
But, the only possibly better key I can think of is:'only_handle_from_transport' => 'events_projection',
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/DependencyInjection/MessengerPass.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
chalasr commentedApr 26, 2019
Maybe |
weaverryan commentedApr 27, 2019
Hmm, I do kinda like |
sroze commentedApr 28, 2019
Happy with |
cc6f13d to271c12bComparesroze commentedApr 28, 2019
This PR should be ready :) |
chalasr left a comment
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.
👍 with minor comment
Uh oh!
There was an error while loading.Please reload this page.
271c12b tof0b2acdCompare…transport (sroze)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] Allows to register handlers on a specific transport| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#30110| License | MIT| Doc PR |symfony/symfony-docs#11236With the#30008 pull-request in, we can now do the following:```yamlframework: messenger: transports: events: dsn: amqp://guest:guest@127.0.0.1/%2f/events options: queues: 3rdparty: binding_keys: [ 3rdparty ] projection: binding_keys: [ projection ] events_3rdparty: amqp://guest:guest@127.0.0.1/%2f/events?queues[3rdparty] events_projection: amqp://guest:guest@127.0.0.1/%2f/events?queues[projection] routing: 'App\Message\RegisterBet': events```This will bind two queues to the `events` exchange, fantastic:<img width="325" alt="Screenshot 2019-04-07 at 10 26 27" src="https://user-images.githubusercontent.com/804625/55680861-af373580-591f-11e9-8f1e-2d3b6ddba2fd.png">---Now, in this setup, the message will be duplicated within the `3rdparty` & `projection` queues. If you just run the consumer for each transport, it will consume the message and call all the handlers. You can't do different things based on which queue you have consumed the message. **This pull-request adds the following feature:**```phpclass RegisterBetHandler implements MessageSubscriberInterface{ public function __invoke(RegisterBet $message) { // Do something only when the message comes from the `events_projection` transport... } /** * {@inheritdoc} */ public static function getHandledMessages(): iterable { yield RegisterBet::class => [ 'from_transport' => 'events_projection', ]; }}```---In the debugger, it looks like this:<img width="649" alt="Screenshot 2019-04-07 at 10 29 55" src="https://user-images.githubusercontent.com/804625/55680892-1d7bf800-5920-11e9-80f7-853f0201c6d8.png">Commits-------f0b2acd Allows to register handlers on a specific transport (and get rid of this handler alias)
… 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
Tobion commentedMay 11, 2019
The I still can't believe we use arbitrary array values in the suscriber instead of a value object that is self-documenting and can have types. There is even a |
…(sroze)This PR was merged into the 4.3 branch.Discussion----------[Messenger] Add `from_transport` in subscriber's phpdoc| Q | A| ------------- | ---| Branch? | 4.3| Bug fix? | yes| New feature? | ø| BC breaks? | ø| Deprecations? | ø| Tests pass? | yes| Fixed tickets |#30958| License | MIT| Doc PR | øAdd the `from_transport` to the subscriber's phpdoc as it was missed from the original PR.Commits-------6a542cd Add transport in subscriber's phpdoc
This PR was merged into the 4.3 branch.Discussion----------Remove info about the HandlersLocator aliasIt seems that the option to provide an alias to the `HandlersLocator` was removed insymfony/symfony#30958 , but there are still some mentions of it in the documentation.Commits-------fe8dc43 Remove info about the HandlersLocator alias
tony-stark-eth commentedJul 14, 2020
Please add the wonderful example from@sroze to the documentation onhttps://symfony.com/doc/current/messenger.html as a how to, took me some time to find this pull request. :) |
Uh oh!
There was an error while loading.Please reload this page.
With the#30008 pull-request in, we can now do the following:
This will bind two queues to the

eventsexchange, fantastic:Now, in this setup, the message will be duplicated within the
3rdparty&projectionqueues. If you just run the consumer for each transport, it will consume the message and call all the handlers. You can't do different things based on which queue you have consumed the message.This pull-request adds the following feature:In the debugger, it looks like this:
