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

Merged
sroze merged 1 commit intosymfony:masterfromsroze:messenger-locator-on-transport
Apr 28, 2019

Conversation

@sroze
Copy link
Contributor

@srozesroze commentedApr 7, 2019
edited
Loading

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

With the#30008 pull-request in, we can now do the following:

framework:messenger:transports:events:dsn:amqp://guest:guest@127.0.0.1/%2f/eventsoptions: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 theevents exchange, fantastic:
Screenshot 2019-04-07 at 10 26 27


Now, in this setup, the message will be duplicated within the3rdparty &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:

class RegisterBetHandlerimplements MessageSubscriberInterface{publicfunction__invoke(RegisterBet$message)    {// Do something only when the message comes from the `events_projection` transport...    }/**     * {@inheritdoc}     */publicstaticfunctiongetHandledMessages():iterable    {yield RegisterBet::class => ['from_transport' =>'events_projection',        ];    }}

In the debugger, it looks like this:
Screenshot 2019-04-07 at 10 29 55

PGBastien and iosifch reacted with thumbs up emoji
@Koc
Copy link
Contributor

Koc commentedApr 7, 2019
edited
Loading

I see some drawbacks with current configuration:

  • All thisqueues[3rdparty][binding_keys][]=3rdparty&queues[projection][binding_keys][]=projection dsn strings is quite hard to understand and write. And dsn string became longer and longer on each new queue.
  • You should create +1 transport for consuming messages from each queue (events_3rdparty,events_projection)
  • Handler (code level) knows about transport name (config level) - it similar to breaking dependency inversion principe but for config

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 coursemessenger:consume command should be updated to use handler name as argumentmessenger:consume 'App\MessageHandler\RegisterBetHandler'

bigfoot90 reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to thenext milestoneApr 7, 2019
@srozesrozeforce-pushed themessenger-locator-on-transport branch 2 times, most recently from106be1d to1de1cb2CompareApril 7, 2019 12:00
@sroze
Copy link
ContributorAuthor

@Koc I don't see your point as being against this proposed implementation here. The main point is to create thisHandlerDescriptor, which allows us to have additional metadata on a specific handler (following a very long discussion with@webmozart).

In the example given, the handler indeed knows about the transport (it's already knowing aboutthings like thebus and its ownpriority when using theMessageSubscriberInterface). But it allows us to harvest these options (HandlerDescriptor'soptions) from outside now, which can indeed be a YAML configuration.

Regarding the overhead in terms of configuring the exchange and the queue bindings, you could also use theoptions if you don't want it in the DSN and might challenge would here be that actually you'd be better in creating this setup by yourself in RabbitMq. Having to create multiple transports actually simplifies the configuration because we don't need to introduce this extra conceptqueue that you suggested above.

@srozesrozeforce-pushed themessenger-locator-on-transport branch from1de1cb2 toaf34fbbCompareApril 7, 2019 12:42
@sroze
Copy link
ContributorAuthor

(requires CHANGELOG when a few 👍 will be around 😉 )

@sroze
Copy link
ContributorAuthor

Open question: should we only havehandlerName inHandlerDescriptor instead of directly the handler so it can later be serialized if needed?

@weaverryan
Copy link
Member

Makes sense to me - we do this with the bus name stamp and sender name for retry.

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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

@srozesroze changed the title[Messenger] Allows to register handlers on a specific transport (and get rid of this handler alias)[Messenger] Allows to register handlers on a specific transportApr 22, 2019
@srozesrozeforce-pushed themessenger-locator-on-transport branch fromaf34fbb to83758b6CompareApril 22, 2019 16:19
@sroze
Copy link
ContributorAuthor

@nicolas-grekas@weaverryan updated 👍

@srozesrozeforce-pushed themessenger-locator-on-transport branch from83758b6 tocc6f13dCompareApril 22, 2019 16:24
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.

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 theevents_projection transport

But it's just as reasonable to think it means this:

This handler will be sent to theevents_projection transport

But, the only possibly better key I can think of is:'only_handle_from_transport' => 'events_projection',

@chalasr
Copy link
Member

But, the only possibly better key I can think of is: 'only_handle_from_transport' => 'events_projection',

Maybefrom_transport is enough to avoid the confusion?

@weaverryan
Copy link
Member

Hmm, I do kinda likefrom_transport

@sroze
Copy link
ContributorAuthor

Happy withfrom_transport as well. Let me change this 👍

@srozesrozeforce-pushed themessenger-locator-on-transport branch fromcc6f13d to271c12bCompareApril 28, 2019 15:12
@sroze
Copy link
ContributorAuthor

This PR should be ready :)

Copy link
Member

@chalasrchalasr left a comment

Choose a reason for hiding this comment

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

👍 with minor comment

@srozesrozeforce-pushed themessenger-locator-on-transport branch from271c12b tof0b2acdCompareApril 28, 2019 15:17
@srozesroze merged commitf0b2acd intosymfony:masterApr 28, 2019
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
fabpot added a commit that referenced this pull requestMay 11, 2019
… 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
Copy link
Contributor

Thefrom_transport is not documented in theMessageSubscriberInterface.

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 aHandlerDescriptor now but only for internal use...

fabpot added a commit that referenced this pull requestMay 13, 2019
…(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
@srozesroze deleted the messenger-locator-on-transport branchJune 2, 2019 15:49
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestOct 2, 2019
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
Copy link

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. :)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@weaverryanweaverryanweaverryan left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

8 participants

@sroze@Koc@weaverryan@chalasr@Tobion@tony-stark-eth@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp