refactor(general): extendBlobRetentionTime
#4960
Conversation
- use `atomic.Uint32` type instead of `uint32` with atomic functions - rename `cnt` to `extendedCount` for clarity
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the extendBlobRetentionTime function to improve code organization and type safety. The key changes make GetLockingStoragePrefixes() return []blob.ID instead of []string, unexport the main function while providing test access, and modernize the concurrency implementation.
- Changed
GetLockingStoragePrefixes()to return[]blob.IDinstead of[]stringfor better type safety - Made
extendBlobRetentionTimeprivate and added a test helper to access it - Replaced manual goroutine management with
errgroupand upgraded toatomic.Uint32
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| repo/locking_storage.go | Updated GetLockingStoragePrefixes() return type from []string to []blob.ID |
| repo/open.go | Added explicit type conversion to string(prefix) to match the new return type |
| repo/maintenance/blob_retain.go | Refactored to use errgroup, atomic.Uint32, removed unused sync import, made function private |
| repo/maintenance/maintenance_run.go | Updated call to use lowercase extendBlobRetentionTime |
| repo/maintenance/helper_test.go | Added test helper to expose private extendBlobRetentionTime function |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4960 +/- ##
==========================================
+ Coverage 75.86% 78.01% +2.15%
==========================================
Files 470 545 +75
Lines 37301 31360 -5941
==========================================
- Hits 28299 24467 -3832
+ Misses 7071 4849 -2222
- Partials 1931 2044 +113 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
02cdfee to
9c49b07
Compare
| result := &maintenancestats.ExtendBlobRetentionStats{ | ||
| ToExtendBlobCount: toExtend.Load(), | ||
| ExtendedBlobCount: extendedCount.Load(), | ||
| RetentionPeriod: extendOpts.RetentionPeriod.String(), | ||
| } |
There was a problem hiding this comment.
In dry-run mode, ExtendedBlobCount will always be 0 since the extension workers are not started (line 67 check). This is a behavioral change from the original code which returned early with only ToExtendBlobCount set. Consider returning early for dry-run mode to maintain the original behavior and avoid confusion.
Uh oh!
There was an error while loading. Please reload this page.