Skip to content

[Scheduler] add RecurringMessage::getId() and prevent duplicates #49838

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

Merged
merged 1 commit into from
May 10, 2023

Conversation

kbond
Copy link
Member

@kbond kbond commented Mar 27, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR todo

This could allow running specific scheduled tasks manually (via CLI or GUI).

@carsonbot carsonbot added Status: Needs Review Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Scheduler labels Mar 27, 2023
@carsonbot carsonbot added this to the 6.3 milestone Mar 27, 2023
@carsonbot carsonbot changed the title [Scheduler][RFC] add RecurringMessage::getId() and prevent duplicates [Scheduler] add RecurringMessage::getId() and prevent duplicates Mar 27, 2023
@kbond
Copy link
Member Author

kbond commented Mar 27, 2023

This change breaks the current test suite for scheduler. I'd like to get some feedback or alternative ideas before going further.

Some context on my requirement for this: In my app, I have an admin GUI that lists all available scheduled tasks. User's can drill into each task (hence the need for a unique ID) and see a run history which includes memory usage and duration. The schedule name and ID on the ScheduledStamp would allow connecting specific runs with a recurring message.

There's likely other methods I can use to achieve this same system with symfony/scheduler so I'm not dead set on this being added.

@kbond
Copy link
Member Author

kbond commented Mar 27, 2023

There's likely other methods I can use to achieve this same system with symfony/scheduler so I'm not dead set on this being added.

I've played a little with this. I could have my own custom logic to generate an identifier for a RecurringMessage and I can infer the scheduler name from RecievedStamp::$transport (schedule_default or schedule_foo - not ideal, but workable).

But I think I'd need the RecurringMessage added to the ScheduledStamp or the Trigger object as a bare minimum in order to connect runs with a specific RecurringMessage.

Basically, I think we need more context added to the ScheduledStamp but it's not possible with the way it's currently coded.

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Having more information in the stamp does not look good as the component should work without using Messenger.

@kbond
Copy link
Member Author

kbond commented Mar 29, 2023

Having more information in the stamp does not look good as the component should work without using Messenger.

Can save for a followup PR but I was thinking to change MessageGeneratorInterface::getMessages(): iterable<object> to: MessageGeneratorInterface::getMessages(): iterable<RecurringMessage>.

In the stand-alone Scheduler, we'd unwrap the message from RecurringMessage. In the transport, we could add the RecurringMessage to the stamp.

@vtsykun
Copy link
Contributor

vtsykun commented Mar 29, 2023

Correctly I understand that now I can not use the same messages with the different arguments?

            ->add(RM::cron('*/6 * * * *', new Class1('aaa')))
            ->add(RM::cron('*/7 * * * *', new Class1('bbb')))
            ->add(RM::cron('*/7 * * * *',  new Class1('ccc')));

In this case I got an error Duplicated schedule message. - will I need to create toString to avoid duplication restriction?

@kbond
Copy link
Member Author

kbond commented Mar 29, 2023

Correctly I understand that now I can not use the same messages with the different arguments?

CronExpressionTrigger is now \Stringable so those would be unique. I'd like to make TriggerInterface extend \Stringable.

@vtsykun
Copy link
Contributor

vtsykun commented Mar 29, 2023

CronExpressionTrigger is now \Stringable so those would be unique

Not sure that will be good. it means that we will not be able to use two the same tasks if their cron trigger is same.

Maybe more simple use iterable id if need to debug or execute the specific tasks manually, like this

php bin/console debug:scheduler --execute-one=7

image1

@kbond
Copy link
Member Author

kbond commented Mar 29, 2023

it means that we will not be able to use two tasks if their cron trigger is same.

In that case, yes, you'd need to make your message stringable.

Maybe more simple use iterable id if need to debug or execute the specific tasks manually, like this

Indeed, that could be an option. I'd like to track runs in the db so those id's changing whenever a schedule is updated wouldn't be desired. However, if the ScheduledStamp had more context, I could overcome this problem.

fabpot added a commit that referenced this pull request Apr 24, 2023
…e` (kbond)

This PR was merged into the 6.3 branch.

Discussion
----------

[Scheduler] have `TriggerInterface` extend `\Stringable`

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | n/a
| License       | MIT
| Doc PR        | todo

Helps with the output of `debug:schedule` and with #49838.

Commits
-------

60dfc7f [Scheduler] have `TriggerInterface` extend `\Stringable`
@kbond kbond force-pushed the schedule-message-id branch 2 times, most recently from ef446f0 to 5e03491 Compare April 25, 2023 23:05
@kbond
Copy link
Member Author

kbond commented Apr 25, 2023

Rebased, refactored and added the ID to MessageContext and therefore the ScheduledStamp.

To address this concern, I now cast the message to a string if stringable, if not, serialize it, if it can't be serialized, ignore. That example now works without needing Class1 to be stringable.

@kbond kbond force-pushed the schedule-message-id branch 2 times, most recently from a3d4e57 to 2473d1f Compare April 27, 2023 14:32
@fabpot
Copy link
Member

fabpot commented May 10, 2023

@kbond Can you rebase and have a look at the failing tests?

@kbond kbond force-pushed the schedule-message-id branch from 2473d1f to 7e9a3ba Compare May 10, 2023 12:55
@kbond
Copy link
Member Author

kbond commented May 10, 2023

Can you rebase and have a look at the failing tests?

Done.

@fabpot
Copy link
Member

fabpot commented May 10, 2023

Thank you @kbond.

@fabpot fabpot merged commit 0a49ff8 into symfony:6.3 May 10, 2023
@kbond kbond deleted the schedule-message-id branch May 10, 2023 16:05
@fabpot fabpot mentioned this pull request May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) Scheduler Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants