Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[messenger] Adds a stamp to provide a routing key on message publishing #30008

Merged
merged 2 commits into from Apr 6, 2019

Conversation

@G15N
Copy link
Contributor

commented Jan 28, 2019

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#11236

Adds a stamp allowing to set a routing_key at MessageBus::dispatch() level.

$message = (new Envelope('message'))->with(new RoutingKeyStamp('routing_key'));
$bus->dispatch($message);

@G15N G15N requested a review from sroze as a code owner Jan 28, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 28, 2019

@BentCoder

This comment has been minimized.

Copy link

commented Feb 4, 2019

Please correct me if I am wrong. As far as I can see, this PR addresses only the second section of the referencing issue which is "MessageBus::dispatch" where RoutingKeyStamp 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 is an example what example I mean by that if you wonder.

Thanks

@weaverryan weaverryan referenced this pull request Mar 15, 2019

Closed

[Messenger] Making it Shine #30540

30 of 36 tasks complete

@G15N G15N force-pushed the G15N:ticket-29950-bus-dispatch-enhancements branch 2 times, most recently from fb3c047 to 24b96e6 Mar 22, 2019

@G15N G15N force-pushed the G15N:ticket-29950-bus-dispatch-enhancements branch 3 times, most recently from 670d99a to 07d98cd Mar 25, 2019

@sroze
Copy link
Member

left a comment

Can you change these, squash & rebase please?

Show resolved Hide resolved UPGRADE-4.3.md Outdated
Show resolved Hide resolved UPGRADE-5.0.md Outdated
Show resolved Hide resolved src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated
Show resolved Hide resolved src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated

@G15N G15N force-pushed the G15N:ticket-29950-bus-dispatch-enhancements branch from af43833 to 8fddd02 Mar 28, 2019

@weaverryan

This comment has been minimized.

Copy link
Member

commented Mar 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 messenger: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 still do need another PR that allows people to create multiple queues per AMQP transport, each with one or more routing keys.

@weaverryan
Copy link
Member

left a comment

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.

@@ -91,6 +91,7 @@ Messenger

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

This comment has been minimized.

Copy link
@weaverryan

weaverryan Mar 29, 2019

Member

Yes! This also did not make sense to me. The routing_key config should be to bind the queue to that routing key, not which routing_key to send messages to.

Show resolved Hide resolved src/Symfony/Component/Messenger/Transport/AmqpExt/Connection.php Outdated
Show resolved Hide resolved src/Symfony/Component/Messenger/Transport/AmqpExt/AmqpRoutingKeyStamp.php
@weaverryan

This comment has been minimized.

Copy link
Member

commented Mar 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 and nothing 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

This comment has been minimized.

Copy link

commented Mar 29, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

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

This comment has been minimized.

Copy link
Member

commented Mar 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

This comment has been minimized.

Copy link

commented Mar 30, 2019

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

@G15N G15N force-pushed the G15N:ticket-29950-bus-dispatch-enhancements branch 2 times, most recently from c2483f9 to 3ab163a Mar 31, 2019

@weaverryan weaverryan referenced this pull request Apr 6, 2019

Open

[Messenger][4.3] Tracker for changes #11236

1 of 25 tasks complete

@sroze sroze force-pushed the G15N:ticket-29950-bus-dispatch-enhancements branch from 92221b8 to 2bb4b88 Apr 6, 2019

@sroze

sroze approved these changes Apr 6, 2019

Copy link
Member

left a comment

Tested, works well 👍

@Nyholm
Copy link
Member

left a comment

Thank you

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

@sroze sroze force-pushed the G15N:ticket-29950-bus-dispatch-enhancements branch from 5023aff to 04298ea Apr 6, 2019

@sroze

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

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

@weaverryan
Copy link
Member

left a comment

One one minor thing +1

@sroze sroze force-pushed the G15N:ticket-29950-bus-dispatch-enhancements branch from 04298ea to a515635 Apr 6, 2019

@sroze

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Thank you @G15N.

@sroze sroze merged commit a515635 into symfony:master Apr 6, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

sroze added a commit that referenced this pull request Apr 6, 2019

feature #30008 [messenger] Adds a stamp to provide a routing key on m…
…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#11236

Adds 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

This comment has been minimized.

Copy link

commented Apr 6, 2019

Good job. Thanks.

sroze added a commit that referenced this pull request Apr 28, 2019

feature #30958 [Messenger] Allows to register handlers on a specific …
…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#11236

With the #30008 pull-request in, we can now do the following:
```yaml
framework:
    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:**

```php
class 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-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.