-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix maintenance task issues raised in #2005 #2020
base: master
Are you sure you want to change the base?
Conversation
dams
commented
Dec 30, 2023
- share 'last_cleaned_at' between workers and set it at maintenance task start time
- release maintenance lock when maintenance task is finished
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind storing this as UTC timestamp instead of string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |