For upstream/avoid useless cleanup

[dams]

It's impacting performance, and it's redundant with the workers' maintenance task that already calls cleanup

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3c89f9d) 93.84% compared to head (04a4092) 93.82%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2003      +/-   ##
==========================================
- Coverage   93.84%   93.82%   -0.02%     
==========================================
  Files          29       29              
  Lines        3897     3889       -8     
==========================================
- Hits         3657     3649       -8     
  Misses        240      240              

Files	Coverage Δ
rq/registry.py	97.92% <ø> (-0.09%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[selwin]

cleanup() is called within count because sometimes this command is called without any workers running. As far as I know, cleanup() executes very quickly, even in registries with lots of jobs but I do understand it incurs a few extra Redis calls.

If you really need to disable this, I suggest adding a cleanup=True argument, you can then pass in False so it doesn't call cleanup() needlessly.

[dams]

cleanup() is called within count because sometimes this command is called without any workers running.

Ah that's a good point.

As far as I know, cleanup() executes very quickly, even in registries with lots of jobs but I do understand it incurs a few extra Redis calls.

Well, "executes very quickly" doesn't tell me a lot 🙂
The code complexity of cleanup is in O(2*log(N)+3*M) with N the number of jobs in the registry and M the number of expired jobs.
We use RQ with up to millions of jobs, for instance in failed registries. Even if there is nothing to expire, hitting O(2*log(N) every time we call count is not possible, it has a real performance hit, both response time and CPU usage on Redis.
Having a count property that is not in O(1) is very counter intuitive in my opinion.

If you really need to disable this, I suggest adding a cleanup=True argument, you can then pass in False so it doesn't call cleanup() needlessly.

It's not a very usable solution, as count is used as a property, and it's also used when doing len so it's very difficult to prevent developers to trigger needless cleanup calls.

What if instead, count check if there is a worker consuming the queue, for instance using:

if len(worker_registration.get_keys(queue=self)) == 0 :
    self.cleanup()

Would you be happy with that?

[selwin]

It's not a very usable solution, as count is used as a property, and it's also used when doing len so it's very difficult to prevent developers to trigger needless cleanup calls.

Sorry, I forgot that it's a property. In that case do you mind opening a PR that adds a get_job_count() method that returns the number of jobs in registry without calling cleanup()?

Having a count property that is not in O(1) is very counter intuitive in my opinion.
Perhaps, but it returns the correct number everytime and works for 99% users. I think it's ok to design something that works well for the vast majority of users. RQ works well even with millions of jobs in queue/registry, but they probably don't call len() as often as you do.

[dams]

Thanks for your answer, I understand your position.
I think I'll provide a different PR to enable/disable the "cleanup when counting" behaviour at the queue level, but still cleanup when there is no worker associated.
Would that be acceptable?

[selwin]

I think implementing get_job_count() would be simpler, let's do that first and see if that fixes your issue.

[sophcass]

Have come across a problem that is somewhat related to what is being discussed this PR. The cleanup also causes problems when calling get_statistics() using a custom serializer for the queue.

When len() is called here, count calls cleanup() here, which sets the incorrect serializer, resulting in pickling errors.

One solution is to remove this cleanup in count, as is discussed in this PR. Or alternatively, we can initiate the registries with Queue instances in get_statistics() here, which ensures the custom serializer is used.

I can open a PR for the second option if that is the preferred solution. If the first option is preferred, it would be great to get a timeframe of when a fix will be implemented. Let me know!