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] 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

Merged

Conversation

@G15N
Copy link

@G15NG15N commentedJan 28, 2019
edited by weaverryan
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#29950
LicenseMIT
Doc PRsymfony/symfony-docs#11236

Adds a stamp allowing to set arouting_key atMessageBus::dispatch() level.

$message = (newEnvelope('message'))->with(newRoutingKeyStamp('routing_key'));$bus->dispatch($message);

JuGa013, meshenka, and erwan-haquet reacted with thumbs up emoji
@bentcoder
Copy link

bentcoder commentedFeb 4, 2019
edited
Loading

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" whereRoutingKeyStamp feature was suggested. If so, is there a plan to address the first section "AMQP configuration" where we can define multiple routing keys for given queue? There isan example what example I mean by that if you wonder.

Thanks

@weaverryanweaverryan mentioned this pull requestMar 15, 2019
36 tasks
@G15NG15Nforce-pushed theticket-29950-bus-dispatch-enhancements branch 2 times, most recently fromfb3c047 to24b96e6CompareMarch 22, 2019 10:15
@G15NG15Nforce-pushed theticket-29950-bus-dispatch-enhancements branch 3 times, most recently from670d99a to07d98cdCompareMarch 27, 2019 15:19
Copy link
Contributor

@srozesroze left a 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?

@G15NG15Nforce-pushed theticket-29950-bus-dispatch-enhancements branch fromaf43833 to8fddd02CompareMarch 28, 2019 13:16
@weaverryan
Copy link
Member

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 formessenger:consume to consume multiple transports at one time, in order of priority. So, I thought we could possibly solve this problem not by using a single transport with multiple queues, but by creating multiple transports, each with 1 queue. But, it's awkward, and a bit confusing as it sort of "works against" the nature of AMQP.

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.

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.

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.


* `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).
Copy link
Member

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.

@weaverryan
Copy link
Member

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

bentcoder commentedMar 29, 2019
edited
Loading

@bentcoder Can you check out#30770 to make sure it accomplishes what you need and makes sense?

Cheers!

@weaverryan I left a small confirmation question there if you kindly check. Thanks

@G15N
Copy link
Author

if you are interested in finishing this, please let me know and please do!

Yup, I am. I'll grab your last commit and work on top of it from now.

@weaverryan
Copy link
Member

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

@weaverryan I can confirm that it does what I asked for. Thank you guys.

@G15NG15Nforce-pushed theticket-29950-bus-dispatch-enhancements branch 2 times, most recently fromc2483f9 to3ab163aCompareMarch 31, 2019 12:43
Copy link
Contributor

@srozesroze left a comment

Choose a reason for hiding this comment

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

Tested, works well 👍

Copy link
Member

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

Thank you

There are a few BC breaks here. We should take better care of them. At least in the changelog.

@srozesrozeforce-pushed theticket-29950-bus-dispatch-enhancements branch from5023aff to04298eaCompareApril 6, 2019 10:14
@sroze
Copy link
Contributor

@Nyholm great points. I've keptchannel,queue andexchange public (because it's a legit extension point) and added a mention of the BC break on theConnection 👍

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.

One one minor thing +1

@srozesrozeforce-pushed theticket-29950-bus-dispatch-enhancements branch from04298ea toa515635CompareApril 6, 2019 12:27
@sroze
Copy link
Contributor

Thank you@G15N.

@srozesroze merged commita515635 intosymfony:masterApr 6, 2019
sroze added a commit that referenced this pull requestApr 6, 2019
…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
Copy link

Good job. Thanks.

sroze added a commit that referenced this pull requestApr 28, 2019
…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)
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@OskarStarkOskarStarkOskarStark left review comments

@NyholmNyholmNyholm left review comments

@fabpotfabpotfabpot requested changes

@weaverryanweaverryanweaverryan approved these changes

+1 more reviewer

@srozesrozesroze approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

10 participants

@G15N@bentcoder@weaverryan@sroze@bigfoot90@fabpot@OskarStark@Nyholm@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp