8000 [Messenger] Making it Shine · Issue #30540 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] Making it Shine #30540

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.

Sign up for GitHub

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

Jump to bottom
Closed
30 of 36 tasks
weaverryan opened this issue Mar 12, 2019 · 20 comments
Closed
30 of 36 tasks

[Messenger] Making it Shine #30540

weaverryan opened this issue Mar 12, 2019 · 20 comments
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them. Messenger RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@weaverryan
Copy link
Member
weaverryan commented Mar 12, 2019 •
edited
Loading

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

What else?
@weaverryan weaverryan added RFC RFC = Request For Comments (proposals about features that you want to be discussed) Help wanted Issues and PRs which are looking for volunteers to complete them. Messenger labels Mar 12, 2019
@Nyholm
Copy link
Member
Nyholm commented Mar 12, 2019

This is also needed imho #28849

@Toflar
Copy link
Contributor
Toflar 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
Copy link
Member
Nyholm 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
Copy link
Contributor
er1z 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
Copy link
Member Author

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

This was referenced Mar 13, 2019
Merged
Closed
@weaverryan
Copy link
Member Author

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
Copy link
Contributor
er1z 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
Copy link
Member

Please discuss this in another issue.

@f2r
Copy link
Contributor
f2r commented Mar 14, 2019 •
edited
Loading

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

@weaverryan
Copy link
Member Author

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

@antonch1989
Copy link
Contributor

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

@Nyholm
Copy link
Member
Nyholm commented Mar 21, 2019 •
edited
Loading

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

@weaverryan
Copy link
Member Author

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
@fabpot
feature #30557 [Messenger] Worker events + global retry functionality…
6ff1185 
… (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
Copy link
Contributor
Guikingone commented Apr 2, 2019 •
edited
Loading

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

This can really help for tests case.

@fabpot
Copy link
Member
fabpot 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
Copy link
Contributor
Toflar commented Apr 2, 2019

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

@Guikingone
Copy link
Contributor

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 😃
8000

@vasilvestre
Copy link

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

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

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 unpinned this issue May 1, 2019
@rjhllr rjhllr mentioned this issue Nov 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Help wanted Issues and PRs which are looking for volunteers to complete them. Messenger RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0