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

Automatic lock release - fixes #21 #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tbrien
Copy link

@tbrien tbrien commented May 4, 2020

Add a configuration for allowing automatic lock release for a task.
Decision to release a lock is based on celery worker inspection.

Copy link
Owner

@steinitzu steinitzu left a comment

Choose a reason for hiding this comment

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

Thanks. I added a few comments. I'm still trying to wrap my head around how this would work and possible edge cases.

This covers only the case where a task is currently being executed by a worker?

What about when you have a task pending in the queue already? Would this determine the task is not active, release the lock and now you have two duplicate tasks in the queue?

celery_singleton/singleton.py Outdated Show resolved Hide resolved
celery_singleton/inspect_celery.py Outdated Show resolved Hide resolved
celery_singleton/singleton.py Outdated Show resolved Hide resolved
celery_singleton/singleton.py Outdated Show resolved Hide resolved
@tbrien
Copy link
Author

tbrien commented May 12, 2020

Thanks for your precious guidelines !
The PR was updated and now apply every suggested changes of previous review (unicity of a task is now based on its lock ID, retrieved from backend).
The behaviour was also updated for taking into account scheduled tasks (only active tasks where scanned before) and task scanning (active and scheduled) is only made if a lock is found.

Here is a sum up of the new option :
This new option, if set for a given task will check for every submission of this task if a lock is present in the backend. If true, the Celery control API is used for scanning workers for active or scheduled tasks matching the lock ID. If no matching task is found at the moment of task submission, the lock is removed.

Note that there are some edge cases, as you suggested @steinitzu : this may lead to multiple tasks running if a task submitted removes a lock and another task is scheduled before the worker state is updated (task does not appear in scheduled or active tasks).
It could also lead to performance issues if a large amount of tasks are running or scheduled and scanned regularly.

This new behaviour was useful for long running tasks, scheduled at relatively low frequency (every 1-15 minutes), in an environnement where workers are ungracefully restarted for exploitation needs. This may not suite every needs.

@steinitzu
Copy link
Owner

steinitzu commented May 20, 2020

It still seems strange to me to only cover scheduled (ETA) and active tasks.

Cause far as I understand, if your workers are busy and you're queuing up new tasks without scheduling this will always clear the lock and you can add infinite duplicate tasks to the queue.
I know if you want to inspect the queue you have to poll the broker, is there some broker agnostic way to do that?

Otherwise I can live with some occasional race conditions and performance issues with this option as long as they're documented and users are aware of them.

@tbrien tbrien force-pushed the master branch 6 times, most recently from 6451a97 to 11365ae Compare May 29, 2020 10:59
@tbrien
Copy link
Author

tbrien commented May 29, 2020

To my knowledge, there is no way to list all submitted tasks in a broker agnostic way.

I updated the documentation with performances consideration and a warning of the incompatibility of the challenge_lock_on_startup in an environnement where tasks marked with that flag are queued while no worker is available for reserving/running them.

Also, the challenge now scans for reserved tasks and the logic is now described in the README

--- here are some details about worker inspection ---
after some digging in celery documentation for inspecting workers, there is also a way to retrieve task that have been received but not yet processed.
Only a few tasks seems to be "reserved" by a worker.
In a local test with 31 tasks queued (running 10s each), at worker startup (with a concurrency=1), 1 task became active and 5 where reserved. 25 tasks where only known to the broker.

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.

None yet

2 participants