[Messenger] Add `WorkerMetadata` to `Worker` class. #42335

okwinza · 2021-07-31T12:44:30Z

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fixes #37736
License	MIT
Doc PR	-

At the moment, there is no clean way to access the values of transportNames or recently introduced queueNames that the worker was configured with, although such data might be quite useful for logging/monitoring or other tasks.

This PR attempts to fix that by adding a new and extensible way to provide additional information about a particular Worker object.

So far, the following PRs could benefit from this change:

Use case example:

As I developer
When a message was consumed from transport with name async.
And the worker state is idle.
Then I want to reset services.

Before this PR, the only solution not relying on using Reflection API would look like this:

    private $servicesResetter;
    private $receiversName;
    private $actualReceiverName = null;

    public function __construct(ServicesResetter $servicesResetter, array $receiversName)
    {
        $this->servicesResetter = $servicesResetter;
        $this->receiversName = $receiversName;
    }

    public function saveReceiverName(AbstractWorkerMessageEvent $event): void
    {
        $this->actualReceiverName = $event->getReceiverName();
    }

    public function resetServices(WorkerRunningEvent $event): void
    {
        if (!$event->isWorkerIdle() && \in_array($this->actualReceiverName, $this->receiversName, true)) {
            $this->servicesResetter->reset();
        }

        $this->actualReceiverName = null;
    }

    public static function getSubscribedEvents(): array
    {
        return [
            WorkerMessageHandledEvent::class => ['saveReceiverName'],
            WorkerMessageFailedEvent::class => ['saveReceiverName'],
            WorkerRunningEvent::class => ['resetServices'],
        ];
    }

With this PR, one could simply use this to retrieve the transport name.

$event->getWorker()->getWorkerMetadata()->getTransportName() === $this->transportName;

So the whole solution would look like this:

    private $servicesResetter;
    private $receiversName;

    public function __construct(ServicesResetter $servicesResetter, array $receiversName)
    {
        $this->servicesResetter = $servicesResetter;
        $this->receiversName = $receiversName;
    }

    public function resetServices(WorkerRunningEvent $event): void
    {
        $actualTransportName = $event->getWorker()->getWorkerMetadata()->getTransportName();
        if (!$event->isWorkerIdle() || !in_array($actualTransportName, $this->receiversName, true)) {
            return;
        }

        $this->servicesResetter->reset();
    }

    public static function getSubscribedEvents(): array
    {
        return [
            WorkerRunningEvent::class => ['resetServices'],
        ];
    }

carsonbot · 2021-07-31T12:44:32Z

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 5.4 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

carsonbot · 2021-08-01T08:46:04Z

Hey!

I think @alexander-schranz has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

alexander-schranz · 2021-08-02T12:46:48Z

@carsonbot find another a reviewer please.

carsonbot · 2021-08-02T12:47:48Z

@tienvx could maybe review this PR?

okwinza · 2021-08-03T17:58:35Z

Friendly ping @Nyholm.
Could you see any value in such a feature? :)

Guikingone · 2021-08-09T15:00:56Z

Hi 👋🏻

I may ask a strange question but what's the benefits compared to using a getWorkerMetadata(): array?

As an event already receive the Worker, any benefits from using a dedicated class? 🤔

okwinza · 2021-08-09T17:10:54Z

Hi 👋🏻

I may ask a strange question but what's the benefits compared to using a getWorkerMetadata(): array?

As an event already receive the Worker, any benefits from using a dedicated class? 🤔

Hi, indeed, the whole PR might be replaced with one or a couple getters but:

IMO bloating Worker class with getters sounds more like a hack and doesn't seem like a good idea overall, it's better to keep the concerns separated here.
I considered going with getWorkerMetadata(): array but the DX looked much worse to me since it wasn't clear what's being returned without first looking through the code/docs, etc. Not to mention that IDE autocomplete wont work and you won't be able to enforce return types on the language level that way. I know we have psalm for this, but still.

chalasr · 2021-08-09T20:22:17Z

I like the idea of using an object but I think it's fine to make it accessible from the Worker instead of passing it to all worker-aware events.

okwinza · 2021-08-31T13:09:46Z

@chalasr I've refactored the PR according to your suggestions. Should be good to go once the CI is green.

src/Symfony/Component/Messenger/WorkerMetadata.php

okwinza · 2021-09-13T14:02:09Z

Ping @chalasr @derrabus.
Is there anything I could do to make the review easier for you guys? :)

lyrixx

It looks cool.

But one question:

from where you gonna call $worker->getWorkerMetadata() ?

src/Symfony/Component/Messenger/WorkerMetadata.php

okwinza · 2021-09-22T11:20:40Z

It looks cool.

But one question:

from where you gonna call $worker->getWorkerMetadata() ?

Much thanks for the review @lyrixx, appreciate it!
The primary use case is to call it from any worker event subscriber, mostly via WorkerRunningEvent::getWorker() or any other event that contains a reference to a Worker object.

Add tests for WorkerMetadata class Fix WorkerMetadataTest header Access WorkerMetadata directly via getter and make it mutable. Fix CS Refactor WorkerMetadata::set() method to accept array Use null as default value for queueNames instead of false Add missing return type Make fabbot happy

okwinza · 2021-09-22T12:19:02Z

Rebased, squashed & updated the PR description. Should be good to go now 🥳 .

lyrixx · 2021-09-22T14:56:10Z

Thanks for your work on this new feature!

okwinza requested a review from sroze as a code owner July 31, 2021 12:44

carsonbot added the Status: Needs Review label Jul 31, 2021

carsonbot added Feature Messenger labels Jul 31, 2021

okwinza force-pushed the feature/messenger-worker-metadata branch 2 times, most recently from c267c7c to 25902aa Compare July 31, 2021 17:34

OskarStark changed the title ~~[Messenger] Add WorkerMetadata to worker lifecycle events~~ [Messenger] Add WorkerMetadata to worker lifecycle events Aug 1, 2021

OskarStark added this to the 5.4 milestone Aug 1, 2021

okwinza changed the title ~~[Messenger] Add WorkerMetadata to worker lifecycle events~~ [Messenger] Add WorkerMetadata to Worker class. Aug 31, 2021

okwinza force-pushed the feature/messenger-worker-metadata branch from 3c10761 to afeaf60 Compare August 31, 2021 13:04

okwinza mentioned this pull request Aug 31, 2021

[Messenger] Log when worker should stop and when SIGTERM is received #42723

Merged

okwinza commented Aug 31, 2021

View reviewed changes

src/Symfony/Component/Messenger/WorkerMetadata.php Outdated Show resolved Hide resolved

OskarStark requested review from chalasr, Nyholm and derrabus August 31, 2021 14:04

okwinza force-pushed the feature/messenger-worker-metadata branch from 53caaa0 to 20b3222 Compare August 31, 2021 14:31

okwinza force-pushed the feature/messenger-worker-metadata branch 2 times, most recently from 20b3222 to 9260aa6 Compare September 10, 2021 12:21

okwinza mentioned this pull request Sep 22, 2021

[Messenger] Add a way to no ack message automatically #42873

Closed

lyrixx reviewed Sep 22, 2021

View reviewed changes

src/Symfony/Component/Messenger/WorkerMetadata.php Outdated Show resolved Hide resolved

okwinza force-pushed the feature/messenger-worker-metadata branch from ff7520c to 583f85b Compare September 22, 2021 11:37

okwinza mentioned this pull request Sep 22, 2021

[Messenger] Move container resetting after receiver acknowledging #43133

Merged

lyrixx approved these changes Sep 22, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 22, 2021

chalasr approved these changes Sep 22, 2021

View reviewed changes

lyrixx merged commit 99ebc69 into symfony:5.4 Sep 22, 2021

okwinza deleted the feature/messenger-worker-metadata branch September 22, 2021 15:12

This was referenced Nov 5, 2021

Release v5.4.0-BETA1 #43937

Merged

Release v6.0.0-BETA1 #43939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Messenger] Add `WorkerMetadata` to `Worker` class. #42335

[Messenger] Add `WorkerMetadata` to `Worker` class. #42335

okwinza commented Jul 31, 2021 •

edited

Loading

carsonbot commented Jul 31, 2021

carsonbot commented Aug 1, 2021

alexander-schranz commented Aug 2, 2021

carsonbot commented Aug 2, 2021

okwinza commented Aug 3, 2021

Guikingone commented Aug 9, 2021 •

edited

Loading

okwinza commented Aug 9, 2021

chalasr commented Aug 9, 2021

okwinza commented Aug 31, 2021

okwinza commented Sep 13, 2021

lyrixx left a comment

okwinza commented Sep 22, 2021 •

edited

Loading

okwinza commented Sep 22, 2021

lyrixx commented Sep 22, 2021

[Messenger] Add WorkerMetadata to Worker class. #42335

[Messenger] Add WorkerMetadata to Worker class. #42335

Conversation

okwinza commented Jul 31, 2021 • edited Loading

Use case example:

carsonbot commented Jul 31, 2021

carsonbot commented Aug 1, 2021

alexander-schranz commented Aug 2, 2021

carsonbot commented Aug 2, 2021

okwinza commented Aug 3, 2021

Guikingone commented Aug 9, 2021 • edited Loading

okwinza commented Aug 9, 2021

chalasr commented Aug 9, 2021

okwinza commented Aug 31, 2021

okwinza commented Sep 13, 2021

lyrixx left a comment

Choose a reason for hiding this comment

okwinza commented Sep 22, 2021 • edited Loading

okwinza commented Sep 22, 2021

lyrixx commented Sep 22, 2021

[Messenger] Add `WorkerMetadata` to `Worker` class. #42335

[Messenger] Add `WorkerMetadata` to `Worker` class. #42335

okwinza commented Jul 31, 2021 •

edited

Loading

Guikingone commented Aug 9, 2021 •

edited

Loading

okwinza commented Sep 22, 2021 •

edited

Loading