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] Adds a stamp to provide a routing key on message publishing#30008
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
src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/ConnectionTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
bentcoder commentedFeb 4, 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.
Please correct me if I am wrong. As far as I can see, this PR addressesonly the second section of thereferencing issue which is "MessageBus::dispatch" where Thanks |
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/Tests/Transport/AmqpExt/AmqpSenderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
fb3c047 to24b96e6Comparesrc/Symfony/Component/Messenger/Stamp/AmqpExt/RoutingKeyStamp.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/ConnectionTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Stamp/AmqpExt/RoutingKeyStamp.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/AmqpSenderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/AmqpSenderTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Tests/Transport/AmqpExt/ConnectionTest.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
670d99a to07d98cdCompare
sroze 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.
Can you change these, squash & rebase please?
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.
Uh oh!
There was an error while loading.Please reload this page.
af43833 to8fddd02Compareweaverryan commentedMar 29, 2019
Thanks for the PR! Let's chat about this :). This is related to@bentcoder's comment here:#28772 (comment) We can (or will soon) have the ability for So, yea, I think we stilldo need another PR that allows people to create multiple queues per AMQP transport, each with one or more routing keys. |
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.
Just some minor changes! Also, can you rebase from the latest master? We'll see if that clears up the test failures - they look unrelated.
UPGRADE-4.3.md Outdated
| * `Amqp` transport does not throw `\AMQPException` anymore, catch `TransportException` instead. | ||
| * Deprecated the `LoggingMiddleware` class, pass a logger to `SendMessageMiddleware` instead. | ||
| * Deprecated routing key from queue configuration (`queue[routing_key]` in the DSN), use exchange configuration instead (`exchange[routing_key]` in the DSN). |
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.
Yes! This also did not make sense to me. Therouting_key config should be tobind the queue to that routing key, not which routing_key tosend messages to.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
weaverryan commentedMar 29, 2019
I've added the second half of this (many queues per transport) on#30770. @G15N Your work is included over on my PR. However, I did NOT intend to "hijack" your PR. In fact, I'm super busy andnothing would make me happier than for you to pull my 1 new commit back into your branch, and for you to be able to finish everything right here. I realize that this is now bigger than you initially may have thought, but if you are interested in finishing this, please let me know and please do! You can always ping me directly on the Symfony Slack if you have any questions. @bentcoder Can you check out#30770 to make sure it accomplishes what you need and makes sense? Cheers! |
bentcoder commentedMar 29, 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.
@weaverryan I left a small confirmation question there if you kindly check. Thanks |
G15N commentedMar 29, 2019
Yup, I am. I'll grab your last commit and work on top of it from now. |
weaverryan commentedMar 30, 2019
Thank you@G15N! There is one comment on my PR about changing “routing_keys” to “binds” (or something like that), which is probably a good idea. |
bentcoder commentedMar 30, 2019
@weaverryan I can confirm that it does what I asked for. Thank you guys. |
c2483f9 to3ab163aCompareUh 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.
92221b8 to2bb4b88Compare
sroze 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.
Tested, works well 👍
Nyholm left a comment• 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.
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.
Thank you
There are a few BC breaks here. We should take better care of them. At least in the changelog.
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.
Uh oh!
There was an error while loading.Please reload this page.
5023aff to04298eaComparesroze commentedApr 6, 2019
@Nyholm great points. I've kept |
Uh oh!
There was an error while loading.Please reload this page.
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.
One one minor thing +1
src/Symfony/Component/Messenger/Transport/AmqpExt/AmqpReceiver.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
04298ea toa515635Comparesroze commentedApr 6, 2019
Thank you@G15N. |
…essage publishing (G15N, sroze)This PR was merged into the 4.3-dev branch.Discussion----------[messenger] Adds a stamp to provide a routing key on message publishing| Q | A| ------------- | ---| Branch? | master| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#29950| License | MIT| Doc PR |symfony/symfony-docs#11236Adds a stamp allowing to set a `routing_key` at `MessageBus::dispatch()` level.```php$message = (new Envelope('message'))->with(new RoutingKeyStamp('routing_key'));$bus->dispatch($message);```Commits-------a515635 Simply code and rename "configuration" to "options"3151b54 [messenger] AMQP configurable routing key & multiple queues
bentcoder commentedApr 6, 2019
Good job. Thanks. |
…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)
Uh oh!
There was an error while loading.Please reload this page.
Adds a stamp allowing to set a
routing_keyatMessageBus::dispatch()level.