Fix maintenance task issues raised in #2005 #2020
Conversation
dams
commented
Dec 30, 2023
dams
commented
- share 'last_cleaned_at' between workers and set it at maintenance task start time
- release maintenance lock when maintenance task is finished
dams
force-pushed
the
dams/fix_maintenance_task
branch
3 times, most recently
from
3b01892
to
ee4f661
Compare
- share 'last_cleaned_at' between workers and set it at maintenance task start time - release maintenance lock when maintenance task is finished
dams
force-pushed
the
dams/fix_maintenance_task
branch
from
ee4f661
to
aed76e9
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2020 +/- ##
==========================================
- Coverage 93.84% 93.81% -0.04%
==========================================
Files 29 29
Lines 3901 3911 +10
==========================================
+ Hits 3661 3669 +8
- Misses 240 242 +2 ☔ View full report in Codecov by Sentry. |
|
|
||
| def set_last_cleaned_at(last_cleaned_at: datetime, connection: 'Redis'): | ||
| """Sets the last time maintenance task was run.""" | ||
| connection.set('last_cleaned_at', utcformat(last_cleaned_at)) |
There was a problem hiding this comment.
Mind storing this as UTC timestamp instead of string?
| Maintenance tasks should run on first startup or every maintenance | ||
| interval (defaults to 10 minutes). | ||
| """ | ||
| l_c_at = self.last_cleaned_at = self.last_cleaned_at or worker_registration.get_last_cleaned_at(self.connection) |
There was a problem hiding this comment.
If we're storing the last cleaning time at self.last_cleaned_at, we don't need l_c_at variable.
There was a problem hiding this comment.
l_c_at is there just for having a shorter line below
|
Actually I just realized something, the cleaning timestamp has to be done on a per queue basis and not RQ wide. It could be that we have different workers working on different queues. |
Unless I did it wrong, my changes store the cleaning timestamp per worker and the cleaning will be done on all the queues of this worker. It's not a RQ wide timestamps. So I think it'll work just fine |
If I have 10 workers running |
Please also write a test for this scenario. |
