Add an AsyncSwitchMessageRouter
Closed this issue · 17 comments
Use case taken from the chat:
first round: http request -> command bus -> custom router checks if command should be handled async (marker interface) and has no "handled-by-queue" metadata set -> route to bernard -> bernard producer add command to queue
second round: bernard consumer -> $command->withAddedMetadata('handled-by-queue', $queueName); -> command bus -> custom router sees the metadata key and now routes to command handler instead of bernard -> command handler -> ...
Idea:
the AsyncSwitchMessageRouter can decorate a "standard router plugin" configured with the in-process message routes. The router should check either a specific message metadata key or if message implements an AsyncMessage marker interface (tbd) and route the message to a configured message producer.
In the draft from the chat the consumer adds a metadata key to tell the router that the message was consumed from a queue. But the router should add such a metada key BEFORE it routes the message to a producer. So the router can check its own metadata key when it receives the message a second time. If the metadata key is present the AsyncSwitchMessageRouter delegates routing to its inner (decorated) router.
@codeliner I have written a router decorator that routes to an async provider or if its already been routed drops through to the decorated router to be handled.
Before I finish the tests, please can you take a look as see if this is what you had in mind. also any suggestions gratefully relieved. I have used this in my project and seems to work as expected.
interface...wonders if they should be in the Async folder? but also its part of the router.
https://github.com/guyradford/service-bus/blob/async-router/src/Plugin/Router/AsyncMessage.php
The switch router decorator
https://github.com/guyradford/service-bus/blob/async-router/src/Plugin/Router/AsyncSwitchMessageRouter.php
Thank you
Guy
@guyradford great work 👍 , exactly what I had in mind
please, submit a PR as soon as your tests are ready. @prolic can take a second look then
Can we remove the if statement here?: https://github.com/guyradford/service-bus/blob/master/src/Plugin/Router/AsyncSwitchMessageRouter.php#L79
and just do:
if ($message instanceof AsyncMessage && !($message->metadata()['handled-by-async-queue'] ?? false))// TODO: do I need to re add the message back into the ActionEvent?
Yes, you need to add the (new) message, because the message is immutable and withAddedMetadata returns a new message (aka reference)
And I'd love to see the AsyncMessagemarker interface in the Prooph\ServiceBus\Async namespace, because the interface can be useful for other scenarios, too. /cc @prolic
Last but not least: a short description of the router would be nice here: https://github.com/guyradford/service-bus/blob/master/docs/plugins.md#routers
Again, many many thanks for the awesome work!
Great stuff. Only one thing besides those comments by @codeliner:
The name handled-by-async-queue is not optimal. That it's Handler async does not mean it has to be some queue involved.
One thing I don't really like is that I need all messages to be handled by the same producer with this router. As soon as I need different async message producers on the same bus it all makes no sense any more, because then I can route those messages directly to their producer in the bus config.
good point @prolic
I need all messages to be handled by the same producer with this router.
definitely a limitation, we should add this as a note in the docs. Using the router is optional so users can decide what they prefer.
and maybe just handled-async then?
@prolic If I understand correctly... because we only have one marker interface and that is hard coded in the router, then we are limited? We could pass in the interface to allow people to chose which interface trigger this?
@guyradford the router knows only one message producer. that is the limitation.
@codeliner exactly. Second: I like handled-async
@codeliner @prolic Another thought...what happens when I add more then one router to the commandBus pluggins? Does it keep all? If so can I guarantee the order?
Should this then be a Router, but then I guess it will also be handled by the other routers?
@guyradford the router knows only one message producer. that is the limitation.
then the Async Routers decorators could be stacked each one had its own marker and producer?
or do u mean it should be able to route a message to many async producers?
@guyradford in theory you could work with priorites the ActionEventDispatcher is aware of such a concept but priorites add too much complexity.
I'd say we keep this AsyncSwitchMessageRouter as simple as it is. I'm fine with the limitiation because normally you'll have one messaging system in place. If not, you should be very explicit with your message routing anyway and therefor use dedicated message -> message producer routes then.
Agree with @codeliner
Hi @codeliner & @prolic
https://github.com/guyradford/service-bus/tree/async-router
Tests done, please can you check before I do a PR
I couldn't use the ?? in the if as that is PHP7 only.
Thank you
Just submit the PR. It is easier to review then. If you need to change something you can simply commit and push to your branch again. The PR gets updated automatically.
gesendet von meinem Nokia 3310
-------- Original message --------
Subject: Re: [prooph/service-bus] Add an AsyncSwitchMessageRouter (#128)
From: Guy Radford notifications@github.com
To: prooph/service-bus service-bus@noreply.github.com
CC: Alexander Miertsch kontakt@codeliner.ws,Mention mention@noreply.github.com
Hi @codeliner & @prolic
https://github.com/guyradford/service-bus/tree/async-router
Tests done, please can you check before I do a PR
I couldn't use the ?? in the if as that is PHP7 only.
Thank you
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
ok ta :)