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] New messenger:stop-workers Command #30754

Merged
merged 1 commit into from Mar 31, 2019

Conversation

Projects
None yet
6 participants
@weaverryan
Copy link
Member

commented Mar 28, 2019

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#11236

o/ me again.

This requires and is built on top of #30708

When 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!

@weaverryan weaverryan requested a review from sroze as a code owner Mar 28, 2019

@weaverryan weaverryan changed the title [WIP][Messenger] New messenger:stop-workers Command [Messenger] New messenger:stop-workers Command Mar 28, 2019

@@ -36,6 +36,10 @@
<tag name="cache.pool" />
</service>

<service id="cache.messenger" parent="cache.app" public="false">

This comment has been minimized.

Copy link
@weaverryan

weaverryan Mar 28, 2019

Author Member

I'm using cache.app because we need this value to be shared amongst multiple servers and it should persist through deploys. However, there is no precedent that I know if for the core to use cache.app. And also, perhaps the service id should be more specific to the "messenger restart" - and not just cache.messenger?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 31, 2019

Member

Can't we use a bus instead here to broadcast to all workers?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 31, 2019

Member

If we keep a cache pool, it should be a dedicated one btw, extending from cache.app.

This comment has been minimized.

Copy link
@sroze

sroze Mar 31, 2019

Member

Can't we use a bus instead here to broadcast to all workers?

It means we'd have to build a broadcast feature, which we don't have nor plan (yet?) to have.

This comment has been minimized.

Copy link
@weaverryan

weaverryan Mar 31, 2019

Author Member

I've made the cache service id more specific - still extends from cache.app like before

@weaverryan weaverryan force-pushed the weaverryan:stop-workers-command branch from c0fc67e to 6ea1002 Mar 28, 2019

@yceruto

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

Recently messenger:consume command was renamed/shortened, so I wonder if this one could be named messenger:stop instead?

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 30, 2019

#30708 has been merged now, so this one can be rebased.

@nicolas-grekas nicolas-grekas added this to the next milestone Mar 31, 2019

@weaverryan weaverryan force-pushed the weaverryan:stop-workers-command branch from 6ea1002 to be03f89 Mar 31, 2019

@weaverryan

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

I've kept the longer command name here, because you won't type it often (it's a command to run at deploy mostly) and I think the longer name makes it a bit more descriptive.

This is ready for review! It does actually work locally ;)

@fabpot

fabpot approved these changes Mar 31, 2019

@fabpot fabpot force-pushed the weaverryan:stop-workers-command branch from be03f89 to 5897162 Mar 31, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Thank you @weaverryan.

@fabpot fabpot merged commit 5897162 into symfony:master Mar 31, 2019

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor was unable to build non-mergeable pull request
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 31, 2019

feature #30754 [Messenger] New messenger:stop-workers Command (weaver…
…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#11236

o/ me again.

This requires and is built on top of #30708

When 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
@@ -36,6 +36,10 @@
<tag name="cache.pool" />
</service>

<service id="cache.messenger.restart_workers_signal" parent="cache.app" public="false">

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 31, 2019

Member

public=false should be removed

$io = new SymfonyStyle($input, $output instanceof ConsoleOutputInterface ? $output->getErrorOutput() : $output);
$cacheItem = $this->restartSignalCachePool->getItem(StopWhenRestartSignalIsReceived::RESTART_REQUESTED_TIMESTAMP_KEY);
$cacheItem->set(time());

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 31, 2019

Member

I'd suggest using microtime(true) everywhere

$this->decoratedWorker->stop();
}
private function shouldRestart(int $workerStartedAt)

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Mar 31, 2019

Member

float for microtime()

nicolas-grekas added a commit that referenced this pull request Mar 31, 2019

minor #30800 [Messenger] fix review (nicolas-grekas)
This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger] fix review

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Fixes my review left in #30754
/cc @weaverryan FYI

Commits
-------

a3de902 [Messenger] fix review
@@ -83,6 +83,9 @@
<argument type="collection" /> <!-- Receiver names -->
<argument type="service" id="messenger.retry_strategy_locator" />
<argument type="service" id="event_dispatcher" />
<call method="setCachePoolForRestartSignal">

This comment has been minimized.

Copy link
@sroze

sroze Mar 31, 2019

Member

Why is this one a call while it's an argument on the other one? 🤔

@weaverryan weaverryan deleted the weaverryan:stop-workers-command branch Mar 31, 2019

fabpot added a commit that referenced this pull request Mar 31, 2019

minor #30802 [Messenger] Add `psr/cache` on Messenger's dependencies …
…(sroze)

This PR was merged into the 4.3-dev branch.

Discussion
----------

[Messenger] Add `psr/cache` on Messenger's dependencies

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #30754
| License       | MIT
| Doc PR        | ø

Commits
-------

0b5004f Add `psr/cache` on Messenger's dependencies

@nicolas-grekas nicolas-grekas removed this from the next milestone Apr 30, 2019

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

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.