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] AddWorkerMetadata toWorker class.#42335

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

Conversation

@okwinza
Copy link
Contributor

@okwinzaokwinza commentedJul 31, 2021•
edited
Loading

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?no
TicketsFixes#37736
LicenseMIT
Doc PR-

At the moment, there is no clean way to access the values oftransportNames or recently introducedqueueNames 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 particularWorker 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 nameasync.
  • And the worker state isidle.
  • 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;publicfunction__construct(ServicesResetter$servicesResetter,array$receiversName)    {$this->servicesResetter =$servicesResetter;$this->receiversName =$receiversName;    }publicfunctionsaveReceiverName(AbstractWorkerMessageEvent$event):void    {$this->actualReceiverName =$event->getReceiverName();    }publicfunctionresetServices(WorkerRunningEvent$event):void    {if (!$event->isWorkerIdle() &&\in_array($this->actualReceiverName,$this->receiversName,true)) {$this->servicesResetter->reset();        }$this->actualReceiverName =null;    }publicstaticfunctiongetSubscribedEvents():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;publicfunction__construct(ServicesResetter$servicesResetter,array$receiversName)    {$this->servicesResetter =$servicesResetter;$this->receiversName =$receiversName;    }publicfunctionresetServices(WorkerRunningEvent$event):void    {$actualTransportName =$event->getWorker()->getWorkerMetadata()->getTransportName();if (!$event->isWorkerIdle() || !in_array($actualTransportName,$this->receiversName,true)) {return;        }$this->servicesResetter->reset();    }publicstaticfunctiongetSubscribedEvents():array    {return [            WorkerRunningEvent::class => ['resetServices'],        ];    }

jakobklippel, murtukov, ihavenoface, okwinza, and upyx reacted with thumbs up emojiOskarStark reacted with eyes emoji
@carsonbot
Copy link

Hey!

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

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://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

@okwinzaokwinzaforce-pushed thefeature/messenger-worker-metadata branch 2 times, most recently fromc267c7c to25902aaCompareJuly 31, 2021 17:34
@carsonbot
Copy link

Hey!

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

Cheers!

Carsonbot

@OskarStarkOskarStark changed the title[Messenger] Add WorkerMetadata to worker lifecycle events[Messenger] AddWorkerMetadata to worker lifecycle eventsAug 1, 2021
@OskarStarkOskarStark added this to the5.4 milestoneAug 1, 2021
@alexander-schranz
Copy link
Contributor

@carsonbot find another a reviewer please.

okwinza reacted with thumbs up emoji

@carsonbot
Copy link

@tienvx could maybe review this PR?

@okwinza
Copy link
ContributorAuthor

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

@Guikingone
Copy link
Contributor

Guikingone commentedAug 9, 2021•
edited
Loading

Hi 👋🏻

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

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

@okwinza
Copy link
ContributorAuthor

Hi 👋🏻

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

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

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

  1. IMO bloatingWorker 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.
  2. I considered going withgetWorkerMetadata(): 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 reacted with thumbs up emoji

@chalasr
Copy link
Member

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.

Guikingone reacted with thumbs up emoji

@okwinzaokwinza changed the title[Messenger] AddWorkerMetadata to worker lifecycle events[Messenger] AddWorkerMetadata toWorker class.Aug 31, 2021
@okwinzaokwinzaforce-pushed thefeature/messenger-worker-metadata branch from3c10761 toafeaf60CompareAugust 31, 2021 13:04
@okwinza
Copy link
ContributorAuthor

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

@okwinzaokwinzaforce-pushed thefeature/messenger-worker-metadata branch from53caaa0 to20b3222CompareAugust 31, 2021 14:31
@okwinzaokwinzaforce-pushed thefeature/messenger-worker-metadata branch 2 times, most recently from20b3222 to9260aa6CompareSeptember 10, 2021 12:21
@okwinza
Copy link
ContributorAuthor

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

Copy link
Member

@lyrixxlyrixx left a comment

Choose a reason for hiding this comment

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

It looks cool.

But one question:

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

okwinza reacted with thumbs up emoji
@okwinza
Copy link
ContributorAuthor

okwinza commentedSep 22, 2021•
edited
Loading

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 viaWorkerRunningEvent::getWorker() or any other event that contains a reference to a Worker object.

Add tests for WorkerMetadata classFix WorkerMetadataTest headerAccess WorkerMetadata directly via getter and make it mutable.Fix CSRefactor WorkerMetadata::set() method to accept arrayUse null as default value for queueNames instead of falseAdd missing return typeMake fabbot happy
@okwinza
Copy link
ContributorAuthor

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

@lyrixx
Copy link
Member

Thanks for your work on this new feature!

okwinza and derrabus reacted with thumbs up emojiokwinza and derrabus reacted with hooray emoji

@lyrixxlyrixx merged commit99ebc69 intosymfony:5.4Sep 22, 2021
@okwinzaokwinza deleted the feature/messenger-worker-metadata branchSeptember 22, 2021 15:12
This was referencedNov 5, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@lyrixxlyrixxlyrixx approved these changes

@chalasrchalasrchalasr approved these changes

@srozesrozeAwaiting requested review from sroze

@NyholmNyholmAwaiting requested review from Nyholm

@derrabusderrabusAwaiting requested review from derrabus

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

Missing transporter name on messenger events

7 participants

@okwinza@carsonbot@alexander-schranz@Guikingone@chalasr@lyrixx@OskarStark
[8]ページ先頭
©2009-2025 Movatter.jp