Clear CFF Uploads as they are overwritten
#745
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #745 +/- ##
=======================================
Coverage 98.02% 98.02%
=======================================
Files 438 438
Lines 36283 36305 +22
=======================================
+ Hits 35566 35588 +22
Misses 717 717
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is ✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #745 +/- ##
=======================================
Coverage 98.02% 98.02%
=======================================
Files 438 438
Lines 36283 36305 +22
=======================================
+ Hits 35566 35588 +22
Misses 717 717
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
@@ Coverage Diff @@
## main #745 +/- ##
=======================================
Coverage 98.02% 98.02%
=======================================
Files 438 438
Lines 36283 36305 +22
=======================================
+ Hits 35566 35588 +22
Misses 717 717
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #745 +/- ##
=======================================
Coverage 98.02% 98.02%
=======================================
Files 438 438
Lines 36283 36305 +22
=======================================
+ Hits 35566 35588 +22
Misses 717 717
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Swatinem
force-pushed
the
swatinem/clear-cff-uploads
branch
from
bd39c1f
to
aadcb43
Compare
|
|
||
|
|
||
| @sentry_sdk.trace | ||
| def delete_uploads_by_sessionid(upload: Upload, session_ids: list[int]): |
There was a problem hiding this comment.
3 things here:
- I could see this taking some time because some customers have a gargantuan number of CFFs in their commits, so this might benefit from making a standalone task - unless we're wanting to do this synchronously. In which case, we'd take a bit extra time processing, so something to contemplate/measure in some way if we can
- How could this react with other sessions being processed in parallel? Not sure if while deleting we could run into errors if two or more processes are trying to dleete the same entry
- How could we test this in practice? I just want to make sure we aren't silently corrupting data by not acknowledging our reports were using data from cff's we deleted or something like that
There was a problem hiding this comment.
- not quite sure the overhead of a separate task (both complexity and runtime) is reasonable for that. but you are right that this might be quite slow. However it is limited to only those uploads/sessions which have been overridden by an upload, so that should be rather limited.
- we are behind the "modify report" (upload processing) lock, so there is no parallelism here. also concurrent deletes shouldn’t be a problem either way, as we filter things by
IN, which sql just ignores when no matching rows exist. - good point, I don’t have a good idea for this yet. Though this change aims to align data in SQL with data inside of the
report_json. So this is actually fixing a pre-existing "data corruption" if you think about it that way.
with these questions, you have brought up another good point: do we need to care about SQL-level locking here? I don’t think we need to, as the deletes are only locking the effected rows for the duration of the transaction (which I guess spans the whole task?), and those particular rows are only touched behind the upload processing lock.
There was a problem hiding this comment.
Assuming the deletions take place a short amount of time, I don't think we'd need to worry
Swatinem
force-pushed
the
swatinem/clear-cff-uploads
branch
from
aadcb43
to
2b7fdda
Compare
There was a problem hiding this comment.
When merged, lets keep a close look at how the DB and customers react to the data (e.g. be aware if there's any huge complains from our customers, if we hear anything from support, etc). I'd post a brief message in the eng/support channel to explain the change and have everyone ready
Swatinem
force-pushed
the
swatinem/clear-cff-uploads
branch
from
2b7fdda
to
ea42b7f
Compare
The `Upload`s in the database should mirror the `Session`s within a stored `Report`. However, so far this was not the case for carryforwarded sessions/uploads. The `Session`s were removed from the `Report` as they were being overridden by new uploads, but that was not the case for the `Upload` rows in the database which were created from the carry-forwarded `Session`s on initial `Report` carry-forwarding. This change will now make sure that the `Upload`s in the database match the `Session`s in the `Report`.
Swatinem
force-pushed
the
swatinem/clear-cff-uploads
branch
from
ea42b7f
to
de2c496
Compare
The
Uploads in the database should mirror theSessions within a storedReport.However, so far this was not the case for carryforwarded sessions/uploads. The
Sessions were removed from theReportas they were being overridden by new uploads, but that was not the case for theUploadrows in the database which were created from the carry-forwardedSessions on initialReportcarry-forwarding.This change will now make sure that the
Uploads in the database match theSessions in theReport.