Delete dailies after rolling up #143
Conversation
Co-authored-by: rowanseymour <[email protected]>
…r handling Co-authored-by: rowanseymour <[email protected]>
Co-authored-by: rowanseymour <[email protected]>
Co-authored-by: rowanseymour <[email protected]>
Co-authored-by: rowanseymour <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
==========================================
+ Coverage 58.03% 58.57% +0.53%
==========================================
Files 7 7
Lines 908 968 +60
==========================================
+ Hits 527 567 +40
- Misses 276 288 +12
- Partials 105 113 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rowanseymour
force-pushed
the
delete-dailies-after-rolling-up
branch
from
84e0ccf to
8550aea
Compare
rowanseymour
force-pushed
the
delete-dailies-after-rolling-up
branch
from
8550aea to
c2fe571
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements automatic deletion of daily archives after they have been rolled up into monthly archives and their records deleted. This helps reduce storage costs by removing redundant daily archives once their data has been consolidated into monthly archives.
Key changes:
- Added
DeleteRolledUpDailyArchivesfunction to identify and delete daily archives that have been rolled up and deleted - Added
DeleteS3Filesutility function for efficient batch deletion of S3 objects (up to 1000 per request) - Integrated the deletion logic into the main archive workflow
- Added comprehensive test coverage for the new deletion functionality
- Updated test data to reflect proper archive states
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| archives/archives.go | Adds DeleteRolledUpDailyArchives function and integrates it into the ArchiveOrg workflow to clean up daily archives after rollup |
| archives/s3.go | Adds DeleteS3Files function for batch deletion of S3 objects with automatic batching and error handling |
| archives/archives_test.go | Adds comprehensive test for the daily archive deletion flow including rollup, record deletion, and archive cleanup |
| testdb.sql | Updates test data to set one archive's needs_deletion to FALSE to ensure proper test conditions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| slog.Error("error deleting S3 object in batch", "bucket", bucket, "key", *e.Key, "error", *e.Message) | ||
| } | ||
| totalDeleted += len(batch) - len(output.Errors) | ||
| lastErr = fmt.Errorf("%d errors deleting S3 objects", len(output.Errors)) |
There was a problem hiding this comment.
The error message only reflects the count of errors from the last batch with failures, not the cumulative count across all batches. If multiple batches have errors, only the last batch's error count is reported in the returned error. Consider tracking and reporting the total number of errors across all batches, or clarifying in the message that this is the last batch's error count.
| // check for individual errors | ||
| if len(output.Errors) > 0 { | ||
| for _, e := range output.Errors { | ||
| slog.Error("error deleting S3 object in batch", "bucket", bucket, "key", *e.Key, "error", *e.Message) |
There was a problem hiding this comment.
Potential nil pointer dereference. The code assumes that e.Key and e.Message are non-nil, but the AWS SDK types may return nil pointers for these fields in error cases. Consider adding nil checks before dereferencing these pointers to avoid panics.
| slog.Error("error deleting S3 object in batch", "bucket", bucket, "key", *e.Key, "error", *e.Message) | |
| key := "<nil>" |
of deleted_on since empty ones don't get deleted_on set
rowanseymour
force-pushed
the
delete-dailies-after-rolling-up
branch
from
a53be61 to
33b267f
Compare
No description provided.