refactor(general): add epoch.Manager.MaybeGenerateRangeCheckpoint #3727
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3727 +/- ##
==========================================
+ Coverage 75.86% 77.14% +1.28%
==========================================
Files 470 476 +6
Lines 37301 28864 -8437
==========================================
- Hits 28299 22268 -6031
+ Misses 7071 4668 -2403
+ Partials 1931 1928 -3 ☔ View full report in Codecov by Sentry. |
julio-lopez
force-pushed
the
fix/maybe-generate-range-checkpoints
branch
from
40414e0
to
b8bff96
Compare
julio-lopez
force-pushed
the
fix/maybe-generate-range-checkpoints
branch
from
f87b9da
to
95b042b
Compare
| // MaybeGenerateRangeCheckpoint may create a new range index for all the | ||
| // individual epochs covered by the new range. If there are not enough epochs | ||
| // to create a new range, then a range index is not created. | ||
| func (e *Manager) MaybeGenerateRangeCheckpoint(ctx context.Context) error { |
There was a problem hiding this comment.
I was wondering if extending the function's return value to include a bool indicating whether a range checkpoint was actually generated, alongside the existing error. This could enhance decision-making in caller functions. Thoughts?
func (e *Manager) MaybeGenerateRangeCheckpoint(ctx context.Context) (/*generated*/bool, error) {
...
There was a problem hiding this comment.
I am not (yet) sure it is needed as part of the actual functionality. The primary caller is "repo maintenance", and maintenance cares about whether or not there was an error in order to stop.
Or what use case did you have in mind?
Either way, we can iterate in a separate PR.
There was a problem hiding this comment.
I just don’t know where this function will be called, but I saw from the code that it either does something or doesn’t do it if the error is nil. I thought that this information might be useful, in any case it would be easier to ignore on the caller side than to change the function later. I would say that this flag answers the question: MaybeYes or MaybeNo? :)
There was a problem hiding this comment.
I think I understand what you are saying.
The Maybe prefixes are meant to indicate (at the call site) that the function may not perform the actual operation at this time, and that's "OK"; because either:
- the system is already in the desired state and no transition / change is needed; or
- it is not necessary that the operation is performed at this time to continue execution.
In the current use case (in a following PR), the caller does not need to know whether or not the operation was executed to make a decision based on that. It DOES need to know if there was an error, because in that case it seems safer to abort.
If it becomes necessary (from the caller) to know whether or not the operation was performed, then yes, we'll add a return value as you suggest.
| @@ -624,6 +643,24 @@ func (e *Manager) maybeGenerateNextRangeCheckpointAsync(ctx context.Context, cs | |||
| }) | |||
| } | |||
|
|
|||
| func getRangeToCompact(cs CurrentSnapshot, p Parameters) (low, high int, compactRange bool) { | |||
| latestSettled := cs.WriteEpoch - numUnsettledEpochs | |||
There was a problem hiding this comment.
(minor readability suggestion) I realize this is just a refactor of existing logic, but perhaps we can make a new helper
(cs *CurrentSnapshot) getLastSettledEpochNumber() int {
return cs.WriteEpoch - numUnsettledEpochs
}and we can reuse that here, as well as in cs.isSettledEpochNumber()?
func (cs *CurrentSnapshot) isSettledEpochNumber(epoch int) bool {
return epoch <= cs.getLastSettledEpochNumber()
}For me it's easier to interpret the significance of that subtraction operation if it's described by a helper function name 🤷
There was a problem hiding this comment.
Makes sense. It can be done in a separate PR.
…3651) Perform index epoch compaction and cleanup during repository maintenance * refactor: rename maintenance task for deleting superseded indexes * maintenance task to cleanup epoch markers * maintenance task to advance write index epoch * maintenance task to compact epoch ranges * quick maintenance task to compact a single (index) epoch * full maintenance task to compact a single (index) epoch Ref: - #3638 - #3639 Followup to: - #3603 - #3645 Related helpers and changes: - #3721 - #3735 - #3709 - #3728 - #3727 - #3726
…opia#3651) Perform index epoch compaction and cleanup during repository maintenance * refactor: rename maintenance task for deleting superseded indexes * maintenance task to cleanup epoch markers * maintenance task to advance write index epoch * maintenance task to compact epoch ranges * quick maintenance task to compact a single (index) epoch * full maintenance task to compact a single (index) epoch Ref: - kopia#3638 - kopia#3639 Followup to: - kopia#3603 - kopia#3645 Related helpers and changes: - kopia#3721 - kopia#3735 - kopia#3709 - kopia#3728 - kopia#3727 - kopia#3726
Ref: