Lock clients. Only one calls the Watchdog. #21
Conversation
| lock_name = os.environ.get('CELERY_SERVERLESS_LOCK_NAME', 'celery_serverless:watchdog') | ||
| lock_url = os.environ.get('CELERY_SERVERLESS_LOCK_URL') | ||
| assert lock_url, 'The CELERY_SERVERLESS_LOCK_URL envvar should be set. Even to "disabled" to disable it.' | ||
|
|
||
| queue_url = os.environ.get('CELERY_SERVERLESS_QUEUE_URL') | ||
| assert queue_url, 'The CELERY_SERVERLESS_QUEUE_URL envvar should be set. Even to "disabled" to disable it.' |
There was a problem hiding this comment.
Letting it break with queue_url = os.environ['CELERY_SERVERLESS_QUEUE_URL'] and raising KeyError might make the code a bit simpler.
There was a problem hiding this comment.
But then it gives less clue about how to solve the problem. "Is it a code error or my mistake?".
To change the message of a KeyError it will get as long as it is now. Is the change worth?
There was a problem hiding this comment.
I would be ok without a specific message. Seeing os.environ['CELERY_SERVERLESS_QUEUE_URL'] on the error is enough for me to know that I am missing that envvar. But whether to change or not is up to you.
There was a problem hiding this comment.
For one line instead of two, I do prefer to put my own error message on this case.
| return invoke_watchdog(with_lock=True, *args, **kwargs) | ||
| finally: | ||
| logger.debug('Releasing the client lock') | ||
| client_lock.release() |
There was a problem hiding this comment.
Since a invoke_watchdog() was just made, we could let the client_lock expire by itself as long as it expires before the lifespan of a watchdog. This could avoid an eventual call by another client. Only works for distributed locks that expire though.
There was a problem hiding this comment.
Yes. One though is to put a timeout instead of unlock the client_lock if the watchdog got invoked with success.
But right now the client lock can be a multiprocessing.Lock if Redis is not available on the client, or is undesirable to flood Redis with lock checks.
What do you think? Do this now or let go for some other PR?
There was a problem hiding this comment.
I think it is ok as it is for now, lets just keep it in mind for future work.
There was a problem hiding this comment.
Issue created for future work: #22
celery_serverless/invoker.py
Outdated
| else: | ||
| assert lock_url, 'The CELERY_SERVERLESS_LOCK_URL envvar should be set. Even to "disabled" to disable it.' | ||
|
|
||
| if default and not lock_url: |
There was a problem hiding this comment.
default_lock? Maybe this variable should always be a string and you could check for multiprocessing, since the redis version is a remote url.
There was a problem hiding this comment.
This born as a copy from _get_watchdog_lock.
Generalizing both will make the default more reasonable...
However, I had not understood your "be a string and check for multiprocessing". You mean receiving lock_url='multiprocessing'?
There was a problem hiding this comment.
Yep, I think it is strange that sometimes it is a class, sometimes a url.
There was a problem hiding this comment.
@sbneto Take a look on how _get_lock is now 👍
celery_serverless/watchdog.py
Outdated
| @@ -23,6 +24,25 @@ | |||
| DEFAULT_STARTED_TIMEOUT = 30 # half minute | |||
|
|
|||
|
|
|||
| def _get_watchdog_lock(lock_url='', lock_name='', default=dummy_threading.Lock, enforce=True) -> '(Lock, str)': | |||
There was a problem hiding this comment.
Since _get_watchdog_lock and _get_client_lock should both work with redis locks, shouldn't their code be shared? It seems to me that there is a reasonable amount of copy/paste between both functions.
There was a problem hiding this comment.
Yes. Both functions should be merged.
This is the 1st test-passing code version anyway ;) Lets get it better before merge the PR.
There was a problem hiding this comment.
Done. Take a look on _get_lock plz
Accepted and tested: 'multiprocessing://' and 'dummy_threading://'
Only one client can call the Watchdog. There is a Client lock to be held. Then it will check for the Watchdog lock. If not held, will invoke a Watchdog.