fix: process flakes task redis lock and key #1101
Conversation
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
📢 Thoughts on this report? Let us know! |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #1101 +/- ##
=======================================
Coverage 97.28% 97.28%
=======================================
Files 454 454
Lines 37355 37405 +50
=======================================
+ Hits 36339 36390 +51
+ Misses 1016 1015 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
tasks/process_flakes.py
Outdated
|
|
||
| except ResponseError as e: | ||
| if "WRONGTYPE" in str(e) and commit_id: | ||
| while redis_client.getdel(list_key) is not None: |
There was a problem hiding this comment.
as the corresponding rpush has a similar compatibility code, this might run into a race condition:
- this task tries to
lpop, runs into an exception - the finisher tries to
lpush, runs into an exception - the finisher completes its "delete + rpush" first
- we then run the
getdelhere
I believe you can avoid this backwards compatibility code by just using a different redis key (though still make sure that the flake_uploads one is being deleted, as I believe it is not defined with a TTL so might hang around for eternity otherwise.
joseph-sentry
force-pushed
the
joseph/fix-flakes
branch
from
178d329 to
52c8d4d
Compare
Previously, I made the decision to use a redis lock to exclude multiple process_flakes tasks from running for the same repo at the same time, so that we would minimize database concurrency issues. The idea was that I would set this redis key that represents "more flakes need to be processed for this repo" and that tasks that failed to acquire the lock could rely on the currently running task to handle their work so they wouldn't have to block. I never actually implemented the part where the currently running task takes the work of tasks that ceded their work to it. This commit implements that by replacing the value of the current redis key with a list of commits that need to be processed, this way the currently running task actually has access to the commits that need to be processed. Since it's possible there are old versions of the ta finisher running, our new process flakes task needs to handle both versions of the redis scheme. If an old process flakes task receives a task queued up by the new task it will do nothing, but the next time a new process flakes task runs for that repo, it will process the flakes for that commit.
joseph-sentry
force-pushed
the
joseph/fix-flakes
branch
from
52c8d4d to
259f76f
Compare
Previously, I made the decision to use a redis lock to exclude multiple process_flakes tasks from running for the same repo at the same time, so that we would minimize database concurrency issues.
The idea was that I would set this redis key that represents "more flakes need to be processed for this repo" and that tasks that failed to acquire the lock could rely on the currently running task to handle their work so they wouldn't have to block. I never actually implemented the part where the currently running task takes the work of tasks that ceded their work to it.
This commit implements that by replacing the value of the current redis key with a list of commits that need to be processed, this way the currently running task actually has access to the commits that need to be processed.