-
Notifications
You must be signed in to change notification settings - Fork 6
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
Remove pulls trigger #450
Remove pulls trigger #450
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #450 +/- ##
==========================================
- Coverage 90.58% 89.97% -0.61%
==========================================
Files 401 324 -77
Lines 12509 9199 -3310
Branches 2103 1633 -470
==========================================
- Hits 11331 8277 -3054
+ Misses 1069 859 -210
+ Partials 109 63 -46
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
39e18e1
to
9f5c4fd
Compare
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.
I’m unsure related to the should_write_to_storage
changes. I don’t think those are needed necessarily?
shared/django_apps/core/models.py
Outdated
if self.state != PullStates.OPEN.value: | ||
# while a pull is OPEN, we check whether to write_to_storage | ||
# when a pull is no longer OPEN, we no longer want the flare in storage. | ||
# The nightly cron job cleans up the value in storage (see FlareCleanupTask in worker) | ||
return False |
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.
this would rather mean that the flare is being stored in the JSON field of the table.
I’m unsure what would happen if you change this value based on the state
. will is copy over data from storage to the database? or will that only happen if you save a new value in the field?
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.
good call - I think I was thinking that if this came back False
it wouldn't be saved anywhere, but you're right, this would just force it onto the db field. Removing.
2e54905
to
96d0b0c
Compare
@@ -147,7 +147,7 @@ def delete_file(self, bucket_name, path): | |||
except ClientError: | |||
raise | |||
|
|||
def delete_files(self, bucket_name, paths=[]): |
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.
This default value is dangerous and unnecessary. fixed the occurrences I found across the project.
741151b
to
2331c4a
Compare
if repository: | ||
self.storage_hash = self.get_archive_hash(repository) | ||
else: | ||
self.storage_hash = None |
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.
Making ArchiveService dependent on a repository was complicating my flare cleanup task.
It seems like repository is only needed to make the hash for reading and writing to the archive - you don't need it to delete.
To make my flare cleanup task much lighter, I removed the requirement for a repository in order to use ArchiveService. I added checks to the ArchiveStorage functions where the hash is used, so that you can't do something like write to storage without a hash.
2331c4a
to
9da9d82
Compare
Part of the quest to reduce locks by eliminating triggers https://github.com/codecov/internal-issues/issues/1029
The trigger is deleted by these changes. The behavior is replaced in a cron job on worker codecov/worker#947.
What the trigger did: every time a pull was updated, if it was transitioning from
OPEN
to any otherstate
, it set theflare
field tonull
.flare
is an Archive field, so the value is either in_flare
on the object or in our Archive storage.Replacement behavior: see
FlareCleanupTask
codecov/worker#947Once the trigger is removed, when the pull transitions from
OPEN
to any otherstate
, leaveflare
alone in order to reduce locks and wait time.The cron job runs overnight, gets all pulls that are non-
OPEN
and have flare (either in our db or in Archive storage), clears it (either in our db or in Archive storage) soflare
isnull
.more about
flare
: