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

[Scheduler] pre_run and post_run events#51805

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

alli83
Copy link
Contributor

@alli83alli83 commentedOct 2, 2023•
edited
Loading

QA
Branch?6.4
Bug fix?no
New feature?yes
Deprecations?no
Tickets#49803 (comment)
LicenseMIT

Based on#49803@kbond and taking into account#51553

The aim of this PR is to be able to act on the accumulated messages 'if something happens' and to be able to recalculate the heap via events (currently pre_run and post_run).
The aim is to have access to

  • the the schedule and therefore add/cancel a certain type of message.
  • MessageContexte (e.g. access the id)
  • The message itself

This PR would complement@Jeroeny#51553 PR by enabling action via events.

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.4" but it seems your PR description refers to branch "6.4 for features".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch 6 times, most recently fromc523449 to70218b2CompareOctober 2, 2023 13:30
@alli83alli83 changed the title[Scheduler] feat: pre_run and post_run events[Scheduler] pre_run and post_run eventsOct 2, 2023
@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch from70218b2 toc90b629CompareOctober 2, 2023 14:17
Copy link
Member

@kbondkbond left a comment

Choose a reason for hiding this comment

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

Quick initial review


namespace Symfony\Component\Scheduler\Event;

final class SchedulerEvents
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure we don't want this class alias system anymore. We just the use event's FQCN.

@@ -99,6 +102,10 @@ public function run(array $options = []): void
if ($queueNames) {
$envelopes = $receiver->getFromQueues($queueNames);
} else {
if (method_exists($receiver, 'getMessageGenerator')) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we'll want the messenger to be aware of the scheduler. Ideally, we want these events dispatched in the scheduler but can't currently think of how.

Copy link
ContributorAuthor

@alli83alli83Oct 2, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Indeed, as discussed, I will add listeners that listens to the WorkerMessageReceivedEvent and to the WorkerMessageHandledEvent and dispatch the corresponding scheduler event. Thank you for your feedback!


class PostRunEvent
{
public function __construct(private readonly MessageGenerator $messageGenerator, private readonly Envelope $envelope)
Copy link
Member

Choose a reason for hiding this comment

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

Assuming we dispatch these events in the scheduler itself, we cannot useEnvelope here as it could be any object.

alli83 reacted with thumbs up emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Indeed the standalone usage

use Symfony\Component\Scheduler\Exception\LogicException;
use Symfony\Contracts\Cache\CacheInterface;

final class Schedule implements ScheduleProviderInterface
{
public function __construct(private readonly EventDispatcherInterface $dispatcher)
Copy link
Member

Choose a reason for hiding this comment

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

This would need to be optional.

alli83 reacted with thumbs up emoji
@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch fromc90b629 to0a0fe36CompareOctober 2, 2023 21:35
$schedulerTransport = $this->receiverLocator->get($event->getReceiverName());
$schedule = $schedulerTransport->getMessageGenerator()->schedule();

$this->eventDispatcher?->dispatch(new PostRunEvent($schedule, $messageContext, $envelope->getMessage()));
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

It seems that I would need the same info for pre_run and post_run event.
I will handle this code duplication.

@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch 3 times, most recently from99f8d96 toc30f5f2CompareOctober 2, 2023 21:53
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.

Just some CS stuff for now.

Comment on lines 23 to 24
public function __construct(private readonly ?EventDispatcherInterface $dispatcher = null)
{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
publicfunction __construct(privatereadonly ?EventDispatcherInterface$dispatcher =null)
{
publicfunction __construct(
privatereadonly ?EventDispatcherInterface$dispatcher =null,
) {

Comment on lines 21 to 22
public function __construct(private readonly ScheduleProviderInterface $schedule, private readonly MessageContext $messageContext, private readonly object $message)
{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
publicfunction __construct(privatereadonlyScheduleProviderInterface$schedule,privatereadonlyMessageContext$messageContext,privatereadonlyobject$message)
{
publicfunction __construct(
privatereadonly ScheduleProviderInterface$schedule,
privatereadonly MessageContext$messageContext,
privatereadonly object$message,
) {

Comment on lines 19 to 20
public function __construct(private readonly ScheduleProviderInterface $schedule, private readonly MessageContext $messageContext, private readonly object $message)
{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
publicfunction __construct(privatereadonlyScheduleProviderInterface$schedule,privatereadonlyMessageContext$messageContext,privatereadonlyobject$message)
{
publicfunction __construct(
privatereadonly ScheduleProviderInterface$schedule,
privatereadonly MessageContext$messageContext,
privatereadonly object$message,
) {

{
public function __construct(
private readonly ContainerInterface $receiverLocator,
private readonly ?EventDispatcherInterface $eventDispatcher = null
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
privatereadonly ?EventDispatcherInterface$eventDispatcher =null
privatereadonly ?EventDispatcherInterface$eventDispatcher =null,

@@ -3,6 +3,7 @@
namespace Symfony\Bundle\FrameworkBundle\Tests\Fixtures\Messenger;

use Symfony\Component\Cache\Adapter\ArrayAdapter;
use Symfony\Component\EventDispatcher\EventDispatcher;
Copy link
Member

Choose a reason for hiding this comment

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

Not used?

->args([
service('messenger.receiver_locator'),
service('event_dispatcher'),
])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
])
])


class PreRunEvent
{
private bool $isCancel = false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private bool$isCancel =false;
private bool$isCancelled =false;

@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch 5 times, most recently from4f8417b to49d909fCompareOctober 3, 2023 10:22
@alli83
Copy link
ContributorAuthor

To access the Schedule, it has been decided to inject the transports via the locator service (messenger.receiver_locator). While it would have been possible to inject the schedules directly via the tagged locatorscheduler.schedule_provider doing so would require memoizing the created schedule to avoid calling getSchedule() twice.

@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch from49d909f tof2cfe76CompareOctober 9, 2023 05:30
@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch 2 times, most recently fromf462be9 toffb8279CompareOctober 10, 2023 06:58
interface MessageGeneratorInterface
{
/**
* @return iterable<MessageContext, object>
*/
public function getMessages(): iterable;

public function getSchedule(): Schedule;
Copy link
Member

Choose a reason for hiding this comment

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

I'm uncomfortable adding this method to the interface as a MessageGenerator implementation might not use a Schedule.
In any case, I don't see a reason to interact with the schedule before or after handling a message; it doesn't seem right (especially as we now - in 6.4 - have other ways) and again a Schedule is not a requirement to send scheduled messages.
I would recommend removing the Schedule from the events.

Copy link
ContributorAuthor

@alli83alli83Oct 11, 2023•
edited
Loading

Choose a reason for hiding this comment

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

This is the case when a message is scheduled to be sent every x hours until an event occurs.
It cannot be processed in the handler (messenger case), because it cannot be already processed at that time. The time at which its state (to be processed or not) can change lies within this x-hour interval. The aim, for example in pre_run, is to be able to check (before sending it to the handler) whether it is still relevant or whether, on the contrary, it should be removed from the stack of messages to be processed (since, for example, it will not have to be sent again if it has been processed in the meantime). The aim is to have access to the schedule and to be able to use the remove method, for example, which will recalculate the messages to be processed (using the setRestart introduced in 6.4).

Alternatively, I could inject the ScheduleProviderInterface directly into the event ?

what I don't understand is that on the Scheduler class side, generators will have a Schedule viaScheduleProviderInterface $scheduleProvider.

Sorry if I misunderstood your point/comment

@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch 3 times, most recently fromffd4881 todc1d61eCompareOctober 12, 2023 03:03
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.

LGTM now (just a few minor comments).

use Symfony\Component\Scheduler\Tests\Messenger\SomeScheduleProvider;
use Symfony\Component\Scheduler\Trigger\TriggerInterface;

class DispatchSchedulerEventListenerTest extends TestCase
Copy link
Member

Choose a reason for hiding this comment

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

Must be moved to the Scheduler component test suite.

->args([
tagged_locator('scheduler.schedule_provider', 'name'),
service('event_dispatcher'),
])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
])
])

@@ -62,8 +66,23 @@ public function run(array $options = []): void

$ran = false;
foreach ($this->generators as $generator) {
foreach ($generator->getMessages() as $message) {
foreach ($generator->getMessages() as $context => $message) {
if ($this->dispatcher) {
Copy link
Member

Choose a reason for hiding this comment

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

What about having testing!$this->dispatcher to have less code in the condition?

use Symfony\Component\Scheduler\Exception\LogicException;
use Symfony\Contracts\Cache\CacheInterface;

final class Schedule implements ScheduleProviderInterface
{
public function __construct(
private readonly ?EventDispatcherInterface $dispatcher = null
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
privatereadonly ?EventDispatcherInterface$dispatcher =null
privatereadonly ?EventDispatcherInterface$dispatcher =null,

Copy link
Member

@dunglasdunglas left a comment

Choose a reason for hiding this comment

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

(when last comments from Fabien will be addressed)

@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch 2 times, most recently fromedecb50 to49f05e3CompareOctober 16, 2023 02:29
@alli83alli83force-pushed thescheduler-events-pre-run-post-run branch from49f05e3 to20fd21aCompareOctober 16, 2023 03:45
@fabpot
Copy link
Member

Thank you@alli83.

@fabpotfabpot merged commit50662d0 intosymfony:6.4Oct 16, 2023
fabpot added a commit that referenced this pull requestOct 16, 2023
This PR was merged into the 6.4 branch.Discussion----------[Scheduler] Fix dev requirements| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | Follow-up of#51805| License       | MITThis should turn the CI greenCommits-------f241ed9 [Scheduler] Fix dev requirements
@alli83alli83 mentioned this pull requestOct 16, 2023
fabpot added a commit that referenced this pull requestOct 17, 2023
This PR was merged into the 6.4 branch.Discussion----------[Scheduler] Add `FailureEvent`| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       || License       | MITFollowing PR#51805, It would be interesting to add a `failureEvent` allowing you, for instance, to remove the recurring message, depending on the error caught.Commits-------891a404 [Scheduler] Add failureEvent
This was referencedOct 21, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@kbondkbondkbond left review comments

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

5 participants
@alli83@carsonbot@fabpot@dunglas@kbond
[8]ページ先頭
©2009-2025 Movatter.jp