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] Making it Shine #30540

Closed
weaverryan opened this issue Mar 12, 2019 · 20 comments

Comments

Projects
None yet
@weaverryan
Copy link
Member

commented Mar 12, 2019

Messenger is great. But, we've only carried it 90% of the way. It's missing key features and key things to really make it as usable as it deserves to be. Similar to #30502, I'd like to ask for help & ideas from the community to really make it shine.

(Note: in some of these cases, we just need more review & testing to help push PR's forward).

  • Validate transport options with OptionsResolver
  • Some sort of decent dashboard (or integration) to get stats about the health of your queues
  • Some support, docs, something, for spinning up multiple workers, restarting them, etc
  • Allow 3rd parties to define virtual transports and the user them maps to real transports
  • Review pending doc PRs
  • Review pending doc issues
  • #30970 Handling & Tracking Failed Messages Support
  • #31288 make BusNameStamp option for migration path
  • #30917 redis adapter
  • #30958 Allows to register handlers on a specific transport
  • #30957 Remove base64_encode from PhpSerializer
  • #30558 (and #29950 #28772) Transport Configuration & priority support (and retry attempts, etc)
  • #29040, #29097 null transport
  • #30020 Calling all handlers, even if one fails
  • #30008 multiple queues & routing key support for AMQP + routing key Envelope
  • #30757 get size of messages waiting in a transport
  • #30670 make sure bus service ids are consistent
  • #30707 Adding argument to MessageBus::dispatch() for envelopes
  • #30756 Fix serialization problem with empty messages
  • #30759 sync transport
  • #30676 dispatch send event not on retry
  • #30708 Move receive loop into Worker and allow supporting multiple transports
  • #30628 transport-by-transport serialization config
  • #29007 Doctrine transport (the Enqueue adapter is great, but we should support some basic adapters in core)
  • #30754 Command to stop/restart all workers
  • #29303 messenger:consume has no output
  • #30557 (#27215 #27008) Retry mechanism
  • #30557 (#29132) Making messenger:consume not fail when a worker fails
  • #30557 Hook on worker failure so it can be handled/logged
  • #30764 log error when messages fail on Worker
  • #30650 Allow stamping messages dispatched by 3rd parties via events
  • #30631 sent received messages to the correct bus automatically
  • #30634 Possibly allow each handler of a message to be sent to a different transport (e.g. 1 handler async, 1 sync) and when received, only the 1 handler is executed.
  • #29476 Command to setup transports
  • #28849 Doctrine transaction support
  • #30671 amqp prefetch

What else?

@Nyholm

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

This is also needed imho #28849

@Toflar

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

What else?

Not sure if this was discussed before (searched GitHub but did not find what I was looking for) but I'm missing priority support. I think this is a crucial missing feature that Enqueue already provides (but not for all adapters). Things like user interaction (such as sending e-mails) should always have higher priority than other jobs so I really like that I can give a message a certain priority. I think with the current stamp concept it might be possible (PriorityStamp) and there are transports that support it natively like e.g. RabbitMQ with x-max-priority. For Redis we could use sorted sets, for DBAL it's just an indexed priority column and an ORDER BY etc.

@Nyholm

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

That is a great point. However, I think it is outside of the scope of the Messenger. We should however document how to achieve that.

Quick howto: You add some AMQP routing metadata on your messages and make sure your transport supports adding the routing key to the AMQP message. Then you can configure your exchange to move some messages to a "high priority queue". Then you need to use a consumer that consumes from a list of queues.

It sounds like a lot, but it is just "implementation details".

@er1z

This comment has been minimized.

Copy link

commented Mar 13, 2019

So far, my list:

  • event listener on failed job (with particular job as an argument); as well as more events just like the Request has. But on failed is more essential,
  • use __invoke result as an output, for example on every succeeded job I want its ID to be written on stdout

@nicolas-grekas nicolas-grekas pinned this issue Mar 13, 2019

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

event listener on failed job (with particular job as an argument); as well as more events just like the Request has. But on failed is more essential,

I've talked a little bit about this here: #29132 (comment)

I tend to agree: the worker should not fail, and we should dispatch some events. Could you open a PR for this?

use __invoke result as an output, for example on every succeeded job I want its ID to be written on stdout

I don't understand this one - you can already fetch the return value from __invoke() - if that's what you need - https://symfony.com/doc/current/messenger/handler_results.html

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I'm missing priority support.

💯 I'd like to support (in all transports) a few common options, like priority, delivery delay, retry attempts, etc. Probably we should create a transport that can support all of these. We should propose this as soon as possible in a PR, because we need to get a few other transports merged AND make them support these before feature freeze at the end of March if we want to get things done for 4.3 (which I do).

I've opened #30558 to discuss and have added to the list.

@er1z

This comment has been minimized.

Copy link

commented Mar 13, 2019

I don't understand this one - you can already fetch the return value from __invoke() - if that's what you need - https://symfony.com/doc/current/messenger/handler_results.html

I mean the result result of __invoke = data written to stdout by worker command or sth.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Please discuss this in another issue.

@f2r

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2019

I hadn't seen this issue, but is my PR could help ? #30454

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

@f2r Just added your PR to my list above. Indeed, I like where you're going with that PR :)

@antonch1989

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

@weaverryan is there something relatively easy I could start with?

@Nyholm

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@antonch1989 how about null transport? Or maybe redis transport?

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2019

Both of those have PR's already. However, reviewing and actually testing those would be an awesome way to start helping :). There aren't a lot of "easy pick" issues right now with this stuff.

fabpot added a commit that referenced this issue Mar 23, 2019

feature #30557 [Messenger] Worker events + global retry functionality…
… (weaverryan)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger] Worker events + global retry functionality

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes, on Messenger only
| Deprecations? | no
| Tests pass?   | NEEDED
| Fixed tickets | #29132, #27008, #27215 and part of #30540
| License       | MIT
| Doc PR        | TODO

This is an alternative to #29132 and #27008. There are several big things:

1) The `messenger:consume` does not die if a handler has an error
2) Events are dispatched before, after and on error a message being handled
3) Logic is moved out of Amqp and into the Worker so that we can have some consistent features, like error handling.
4) A generic retry system was added, which works with Amqp and future transports should support.
 It will work out of the box for users. Retrying works by putting the received `Envelope` back into the bus, but with the `ReceivedStamp` removed. The retry functionality has an integration test for AMQP.
5) Added a new `MessageDecodingFailedException` that transport Serializers should throw if `decode()` fails. It allows us to reject a message in this situation, as allowing it to fail but remain on the queue causes it to be retried forever.
6) A new `DelayStamp` was added, which is the first of (later) more stamps for configuring the transport layer (see #30558).

BC breaks are documented in the CHANGELOG.

Thanks!

Commits
-------

a989384 Adding global retry support, events & more to messenger transport
@Guikingone

This comment has been minimized.

Copy link

commented Apr 2, 2019

Small question, what about adding a filesystem transport in the component? (without using an external library).

This can really help for tests case.

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@Guikingone I would not add such a transport, which is very fragile. We already have Doctrine and in memory, I think that's more than enough.

@Toflar

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

PDO/Doctrine and SQLite basically is filesystem. Why re-inventing the wheel? :)

@Guikingone

This comment has been minimized.

Copy link

commented Apr 2, 2019

Enqueue has both 😄

In fact, the idea was to have a dedicated filesystem transport in order to get rid of Doctrine/RabbitMQ/Redis usage in test env but as fabpot says, the in-memory transport is already in place 😃

@vasilvestre

This comment has been minimized.

Copy link

commented Apr 5, 2019

What do you think about allowing empty "type" headers to allow simpler message?
Use case: Having a queue with one class only that's used to receive a small message and dispatch notification

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

That will be possible in 4.3 like this:

  1. Create a custom serializer that reads in your JSON and converts it into a message. This class could be dead-simple, as you could use the serializer service to deserialize right into whatever object you want.

  2. Set that custom serializer as the serializer for some transport - because in 4.3 it's possible to set serializers on a transport-by-transport basis.

The messenger:consume command will also be able to consume from multiple transports at once, so if you have 5 transports with this style, you can consume them all at one time.

Btw, about point (1), you should also implement the encode(Envelope $envelope) method in your serializer and make it actually properly encode the Envelope/Stamps in a way that your decode() method understands if you want the new, automatic retry mechanism to work. Or, if this message originated from an external system (I'm guessing this is the case for having raw JSON without any type headers), you could configure the message class to "route" to some normal transport that uses the normal serialization. Then, you would consume it through your custom transport, but it would be retried through another transport, which uses normal serialization.

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

Closing this issue as incredible progress has been made on Messenger! Thanks for everyone who helped. Let's continue tracking individual items on new, individual issues.

Cheers!

@weaverryan weaverryan closed this May 1, 2019

@weaverryan weaverryan unpinned this issue May 1, 2019

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.