feat(general): add stats to maintenance run - AdvanceEpoch #4937
Conversation
julio-lopez
left a comment
There was a problem hiding this comment.
see small inline comment, it can be done in a separate PR
There was a problem hiding this comment.
Pull Request Overview
This PR adds observability statistics for the AdvanceEpoch maintenance subtask to improve troubleshooting and help users understand repository behavior. The enhancement provides detailed information about epoch advancement operations, including whether an epoch was actually advanced and the current epoch number.
Key changes:
- Introduces
AdvanceEpochStatsstruct to track epoch advancement metrics - Modifies
MaybeAdvanceWriteEpochto return statistics instead of just an error - Updates all test cases to handle the new return value structure
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| repo/maintenancestats/stats_advance_epoch.go | New file defining the AdvanceEpochStats struct with JSON serialization and summary methods |
| repo/maintenancestats/builder_test.go | Added test cases for serializing and deserializing AdvanceEpochStats |
| repo/maintenancestats/builder.go | Updated BuildFromExtra to handle the new advanceEpochStatsKind |
| repo/maintenance/maintenance_run.go | Modified runTaskEpochAdvance to capture and return statistics from MaybeAdvanceWriteEpoch |
| internal/epoch/epoch_manager_test.go | Updated all test cases to handle new return signature and validate returned statistics |
| internal/epoch/epoch_manager.go | Changed MaybeAdvanceWriteEpoch signature to return AdvanceEpochStats along with error |
| var message string | ||
| if as.WasAdvanced { | ||
| message = fmt.Sprintf("Advanced epoch to %v", as.CurrentEpoch) | ||
| } | ||
|
|
||
| return message |
There was a problem hiding this comment.
The Summary method returns an empty string when WasAdvanced is false. Consider returning a meaningful message like 'No epoch advancement needed' for better user experience and consistency.
| var message string | |
| if as.WasAdvanced { | |
| message = fmt.Sprintf("Advanced epoch to %v", as.CurrentEpoch) | |
| } | |
| return message | |
| if as.WasAdvanced { | |
| return fmt.Sprintf("Advanced epoch to %v", as.CurrentEpoch) | |
| } | |
| return "No epoch advancement needed" |
There was a problem hiding this comment.
This can be addressed in a separate PR.
Let's let CI finish, we can merge once it passes.
There was a problem hiding this comment.
Since the maintenance which are with various sub tasks is already complex, I want to make the the output simple --- if a sub task doesn't make any change, just give no output.
There was a problem hiding this comment.
either way is fine, your call.
| result := &maintenancestats.AdvanceEpochStats{ | ||
| CurrentEpoch: cs.WriteEpoch, | ||
| } | ||
|
|
||
| if shouldAdvance(cs.UncompactedEpochSets[cs.WriteEpoch], p.MinEpochDuration, p.EpochAdvanceOnCountThreshold, p.EpochAdvanceOnTotalSizeBytesThreshold) { | ||
| return errors.Wrap(e.advanceEpochMarker(ctx, cs), "error advancing epoch") | ||
| if err := e.advanceEpochMarker(ctx, cs); err != nil { | ||
| return nil, errors.Wrap(err, "error advancing epoch") | ||
| } | ||
|
|
||
| result.CurrentEpoch++ | ||
| result.WasAdvanced = true | ||
| } |
There was a problem hiding this comment.
The CurrentEpoch field is set to cs.WriteEpoch initially and then incremented when advanced. This creates a dependency on the order of operations. Consider setting CurrentEpoch directly to the final value to make the logic clearer and less error-prone.
There was a problem hiding this comment.
Let me change it to:
result.CurrentEpoch = cs.WriteEpoch + 1
Since the CI has started, I can make this change in the next PR.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4937 +/- ##
==========================================
+ Coverage 75.86% 78.02% +2.15%
==========================================
Files 470 541 +71
Lines 37301 31288 -6013
==========================================
- Hits 28299 24411 -3888
+ Misses 7071 4836 -2235
- Partials 1931 2041 +110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks! |
Maintenance is critical for healthy of the repository.
On the other hand, Maintenance is complex, because it runs multiple sub tasks each may generate different results according to the maintenance policy. The results may include deleting/combining/adding data/metadata to the repository.
It is worthy to add more observability for these tasks for below reasons:
There will be a serial of PRs to add observability info for each sub task.
The current PR add the stats info for AdvanceEpoch sub task.