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

[5.4] add OverlappingStrategy for Schedule/Event #18295

Merged
merged 8 commits into from
Mar 23, 2017
Merged

[5.4] add OverlappingStrategy for Schedule/Event #18295

merged 8 commits into from
Mar 23, 2017

Conversation

fetzi
Copy link
Contributor

@fetzi fetzi commented Mar 10, 2017

The withoutOverlapping implementation in the Event class is not working in all cases. For example when the process gets killed, the cache entry is not removed and the scheduled command will not start again until the cache entry expires.

Therefore I've implemented an OverlappingStrategy Interface to be able to override the standard cache strategy. The interface specifies 3 methods:

  • prevent: prevent overlapping for the current event's command (e.g. writes cache entry)
  • reset: resets the overlapping strategy for the event's command (e.g. deletes cache entry)
  • overlaps: checks if the event's command is overlapping with an already running command (e.g. checks if cache entry exists)

To be compatible with the current implementation I've added a CacheOverlappingStrategy that provides the same functionality as the current implementation.

@taylorotwell
Copy link
Member

So this doesn't actually change the current behavior? What does your custom strategy look like?

@fetzi
Copy link
Contributor Author

fetzi commented Mar 10, 2017

I check if the artisan command is running already by filtering the output of ps aux, I know that this solution is only valid for unix operating systems and therefore the current behavior should be kept.

But I can include my custom strategy in the PR if you want me to?

Copy link

@alain-lf alain-lf left a comment

Choose a reason for hiding this comment

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

Would be nice to be able to modify the expiration of the mutex in configs


public function prevent(Event $event)
{
$this->cache->put($event->mutexName(), true, 1440);
Copy link

@alain-lf alain-lf Mar 11, 2017

Choose a reason for hiding this comment

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

1440 = 24 hours.

Depending on your workload, if one of your servers goes down, you might not want to wait a full day for the key to expire and resume schedule.

@fetzi
Copy link
Contributor Author

fetzi commented Mar 11, 2017

@alain-lf Adding the cache entry lifetime for the overlapping check to the config would be an option, but I think this would be to much change for a minor feature change.

And also a shorter cache entry lifetime does not solve the overlapping problem when the process dies, it only reduces the time until the process can be restarted. Maybe the ProcessOverlappingStrategy would be a solution fror you as well?

@taylorotwell what do you think about the config for the mutex cache entry liftetime?

@taylorotwell
Copy link
Member

You wouldn't really want a global config item for this. It would be job specific.

@fetzi
Copy link
Contributor Author

fetzi commented Mar 14, 2017

That's true, I will skip this for the PR because it is targeting the extensibility of the overlapping strategy. Is the PR ready for merging or do you need additional infos?

fetzi and others added 2 commits March 20, 2017 07:03
Attempt to use atomic cache operation when checking for overlaps.
@fetzi
Copy link
Contributor Author

fetzi commented Mar 21, 2017

@taylorotwell ?

@taylorotwell
Copy link
Member

No need to keep pinging. I will eventually look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants