This repository was archived by the owner on May 5, 2025. It is now read-only.
fix: process flakes task redis lock and key #1101
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. |
Swatinem
approved these changes
Feb 21, 2025
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
Swatinem
approved these changes
Feb 26, 2025
JerrySentry
added a commit
that referenced
this pull request
Mar 12, 2025
joseph-sentry
added a commit
that referenced
this pull request
Mar 14, 2025
we previously were using the method of setting a key in redis to have a value to let the process flakes task know that there was more work to do, however this made it so the process flakes task would skip processing certain commits this is a follow-up change to previous changes we made to the process flakes task: #1101 this change makes it so the test results finisher is using the new method of queueing up commitids in redis before queueing up the process flakes task there will be another change where we make it so process flakes no longer checks the old redis key and that will complete this change
joseph-sentry
added a commit
that referenced
this pull request
Mar 19, 2025
we previously were using the method of setting a key in redis to have a value to let the process flakes task know that there was more work to do, however this made it so the process flakes task would skip processing certain commits this is a follow-up change to previous changes we made to the process flakes task: #1101 this change makes it so the test results finisher is using the new method of queueing up commitids in redis before queueing up the process flakes task there will be another change where we make it so process flakes no longer checks the old redis key and that will complete this change
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.