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] ReceiverInterface::handle() to get() & Worker with prioritized transports#30708

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
fabpot merged 1 commit intosymfony:masterfromweaverryan:receiver-no-handler
Mar 30, 2019

Conversation

@weaverryan
Copy link
Member

@weaverryanweaverryan commentedMar 26, 2019•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsHelps with#30699
LicenseMIT
Doc PRTODO

Highlights:

  • messenger:consume can now consume messages from multiple transports with priority ❗️
bin/console messenger:consume amqp_high amqp_medium amqp_low
  • How long you want to sleep before checking more messages is now an option tomessenger:consume
  • ReceiverInterface::receive() is replaced withReceiverInterface::get()
  • Logic for looping & sleeping is moved intoWorker

yceruto reacted with thumbs up emojiKoc reacted with rocket emoji
}
$handled(null);

usleep(1000000);
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Two changes here:

  1. I changed from .2 seconds to 1 second. Waiting only 200ms between when there is not a message is quite rapid. For reference, in Laravel, this defaults to 3 and is configurable on the command/worker.

@weaverryan
Copy link
MemberAuthor

weaverryan commentedMar 27, 2019•
edited by ogizanagi
Loading

This is ready!

tl;dr In recent changes, theReceiverInterface is not really the layer that's handling the "looping and waiting for messages" layer - theWorker is doing that. This completes that transformation. It will also allow us to create aWorker that listens to multiple queues at once, in a priority order (#30699).

@sroze
Copy link
Contributor

The reasoning of havinghandle($handler) was for the transports to be able to catch the exceptions from the handlers and deal with the errors (i.e. ack, nack, etc...). Since we moved this to the worker, I'm happy to move back to get one message. Though, why wouldn't we return aniterable instead to match the behaviour of prefetching (i.e. some transports would have loaded X messages, not just one)?

@weaverryanweaverryanforce-pushed thereceiver-no-handler branch 2 times, most recently from3864a11 to8866804CompareMarch 28, 2019 03:14
@weaverryanweaverryan changed the title[Messenger] Changes ReceiverInterface::handle() to get()[Messenger] ReceiverInterface::handle() to get() & Worker with prioritized transportsMar 28, 2019
* Stop receiving some messages.
*/
publicfunctionstop():void;
publicfunctionget():iterable;
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

The iterable, like always, was kind of a pain, compared to just returning one. But, itis more flexible and end-users won't touch this method much anyways.

@weaverryan
Copy link
MemberAuthor

This is ready for review! Description updated.

Better viewed (especially theWorker with?w=1). The 3StopWhen*** classes are not new - but were moved & changed enough (they decorateWorkerInterface instead ofReceiverInerface so they look new.

Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

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

I really like this.

I am not a fan of priority in a message system, but this is priority of transports. That is a different thing. Im all 👍
(after my comments are addressed/answered)

Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

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

Good. I’m happy with the updated doc block on ReceiverInterface::get().

Thank you

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

minor typos

@fabpot
Copy link
Member

Thank you@weaverryan.

@fabpotfabpot merged commite800bd5 intosymfony:masterMar 30, 2019
fabpot added a commit that referenced this pull requestMar 30, 2019
…ker with prioritized transports (weaverryan)This PR was squashed before being merged into the 4.3-dev branch (closes#30708).Discussion----------[Messenger] ReceiverInterface::handle() to get() & Worker with prioritized transports| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | Helps with#30699| License       | MIT| Doc PR        | TODOHighlights:* `messenger:consume` can now consume messages from multiple transports with priority ❗️```bin/console messenger:consume amqp_high amqp_medium amqp_low```* How long you want to sleep before checking more messages is now an option to `messenger:consume`* `ReceiverInterface::receive()` is replaced with `ReceiverInterface::get()`* Logic for looping & sleeping is moved into `Worker`Commits-------e800bd5 [Messenger] ReceiverInterface::handle() to get() & Worker with prioritized transports
@fabpot
Copy link
Member

typos fixed inf4176b0

interface ReceiverInterface
{
/**
* Receive some messages to the given handler.
Copy link
Contributor

Choose a reason for hiding this comment

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

This doc is outdated: There is no "given handler"

fabpot added a commit that referenced this pull requestMar 31, 2019
…erInterface::get` phpdoc. (sroze)This PR was merged into the 4.3-dev branch.Discussion----------[Messenger] Remove the mention of handler in the `ReceiverInterface::get` phpdoc.| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#30708 (review)| License       | MIT| Doc PR        | øAs spotted by@Tobion, we don't have an handler as an argument anymore.Commits-------9c63112 Remove the mention of handler in the phpdoc.
@weaverryanweaverryan deleted the receiver-no-handler branchMarch 31, 2019 14:42
fabpot added a commit that referenced this pull requestMar 31, 2019
…ryan)This PR was squashed before being merged into the 4.3-dev branch (closes#30754).Discussion----------[Messenger] New messenger:stop-workers Command| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | Kinda of#29451| License       | MIT| Doc PR        |symfony/symfony-docs#11236o/ me again.This requires and is built on top of#30708When you deploy, all workers need to be stopped and restarted. That's not currently possible, unless you manually track the pids and send a SIGTERM signal. We can make that much easier :).Now run:```bin/console messenger:stop-workers```And it will signal to all workers (even if they're distributed on other servers) that they should stop, once they finish processing their current message. This is done via a key in `cache.app`.Cheers!Commits-------5897162 [Messenger] New messenger:stop-workers Command
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@Tobion
Copy link
Contributor

Just thinking out loud for reference: The prioritized transports can be used to prioritize messages by publishing messages to different transports and consuming them in order. But it's a different way than rabbitmq uses message priority. If you want to use rabbitmq priorities instead, you can do that by setting the priority attribute on theAmqpStamp as introduced in#30913

@fabpotfabpot mentioned this pull requestMay 9, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@chalasrchalasrchalasr left review comments

@fabpotfabpotfabpot approved these changes

@NyholmNyholmNyholm approved these changes

@srozesrozeAwaiting requested review from sroze

+1 more reviewer

@TobionTobionTobion left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

8 participants

@weaverryan@sroze@fabpot@Tobion@Nyholm@chalasr@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp