Feature/add scheduler pid info #570
Conversation
django_rq/utils.py
Outdated
| if _ := p.get(lock_key): | ||
| return scheduler.pid | ||
| else: | ||
| for key in p.keys(f"{scheduler.redis_scheduler_namespace_prefix}*"): | ||
| if not p.hexists(key, 'death'): | ||
| return scheduler.pid |
There was a problem hiding this comment.
For some reasons the lock_key was not always there.
So, the extra check for the scheduler instance.
|
@gabriels1234 do you mind checking the tests and make sure they pass? |
@selwin Thanks, made it so the tests work. 2 tests with --with-scheduler and 2 tests with RQ-Scheduler. (Tests passed via installing rq-scheduler, testing (2 passed, 2 skipped), then uninstalling rq-scheduler, testing (2 passed, 2 skipped)) |
|
@selwin let me know if the tests work for you. (I added 4 new tests, 2 (active/inactive) for --with-scheduler and 2 for rq-scheduler). Not sure if the other tests were working or not, but mine do work. |
|
@selwin Based on the merged PR by dear @cunla (rq/rq#1787), now that RQ proper has the scheduler_pid, should I use that and add it to the logic to get the pid when the scheduler is rq-scheduler? |
It needs to be released prior to that. @selwin, when is it expected to be released? |
Let's just add a note that the current implementation should eventually be replaced with |
django_rq/utils.py
Outdated
| if _ := conn.get(lock_key): | ||
| return scheduler.pid | ||
| else: | ||
| for key in conn.keys(f"{scheduler.redis_scheduler_namespace_prefix}*"): |
There was a problem hiding this comment.
I just realized that running keys() will be very costly in a Redis database with a large number of keys. I think we should avoid doing this and just support the built in RQScheduler.
There was a problem hiding this comment.
Yes, I agree totally. Perhaps making a PR to make sure rq-scheduler sets scheduler_lock_key? because there will be a transition period. for now, I will make it so if no scheduler_lock_key, will return -1 and will be rendered as "Unknown". Will let you know after I finish fixing the PR.
There was a problem hiding this comment.
Finally, I decided to just omit functionality if RQ-Scheduler is being used as per your recommendation.
There was a problem hiding this comment.
See below, functionality was removed for rq-scheduler.
django_rq/tests/tests.py
Outdated
| @@ -808,6 +808,88 @@ def test_force_escape_regular_string(self): | |||
| self.assertEqual(escaped_string, expected) | |||
|
|
|||
|
|
|||
| def mocked_relase_locks(): | |||
|
May I get a screenshot of how this looks like on a queue with scheduler active? |
| try: | ||
| # first try get the rq-scheduler | ||
| scheduler = get_scheduler(queue.name) # should fail if rq_scheduler not present | ||
| return False # Not possible to give useful information without creating a performance issue (redis.keys()) |
There was a problem hiding this comment.
This will cause get_scheduler_pid to always return False if rq_scheduler is installed, even though the built in scheduler is used.
I think we should just skip the check for external scheduler altogether.
There was a problem hiding this comment.
Well, ok. In my tests, running both --with-scheduler and rq-scheduler doesn't end up well.
By returning False, I'm basically disabling the feature, since it won't be shown in the dashboard.
After Some digging I found that, for some reason, rq and rq-scheduler organize scheduling and enqueuing of scheduled jobs in parallel ways, namely incompatible.
Please correct me if I'm wrong: Basically, jobs scheduled using Scheduler.enqueue_at will never be picked up by a worker --with-scheduler (different redis keys), and also the opposite is true. I wouldn't understand anyone having both running (and if they do, the worker will be worker only).
Recently, @cunla's django-rq-scheduler (>2022.12.1) transitioned to --with-scheduler.
My point is: showing that the Scheduler is active (worker --with-scheduler) and then scheduling jobs using rq-scheduler enqueue_at will cause stale jobs that never run and are never seen by django-rq.
What I propose is your original idea but only working if the rq-scheduler is not to be found, otherwise, the information would be misleading, to say the least.
Let me know
There was a problem hiding this comment.
What I propose is your original idea but only working if the rq-scheduler is not to be found, otherwise, the information would be misleading, to say the least.
@selwin as I mentioned, I prefer not to activate the feature for rq-scheduler, because it would be misleading.
I think the PR is ready to move forward. (At least to ship it as a half-feature)
However, I'm happy to make it according to your opinion.
There was a problem hiding this comment.
I agree, let's drop rq-scheduler altogether as it will only make things confusing.
There was a problem hiding this comment.
@selwin great! So the PR is good as it is. do you need to see a screenshot?
There was a problem hiding this comment.
Yes please, a screenshot would be good just to make sure that it works :)
|
@selwin looking forward to hear from you how should I go about. |
|
Sorry for the late reply @gabriels1234 |
|
Thanks! |
|
Thanks! |
Based off @selwin encouragement (#567), This PR adds to the general view the PID of the scheduler.
It handles both rq-scheduler and rq worker --with-scheduler.
it helps visualize whether there's an issue (e.g worker started without scheduler, or rq-scheduler not responding). Since worker-schedulers are queue-specific, it made a lot of sense to add it as a column to the main django-rq view.
It does that by checking scheduler data in redis (getting connection either from the queue or from the scheduler) and considers that rq and rq-scheduler store this differently.
If there's no active scheduler, the column will show "Inactive" (this might take some time since the data in redis has to expire (1~(rq) to 15~ (rq-scheduler) minutes)).
Wrote some tests, but need help in running them (rq has it easier).
PS: (PIDs can be the same but the schedulers can be different (check the name of the worker to confirm they are different))