feat: Add DeleteRange method to Store
#347
Conversation
|
One thing we should explore is making DeleteRange [from:to) instead of introducing a new method. It's gonna have better ergonomics, allow both usecase, and have less duplicated code and easier to maintain longer term. We actually wanted to make it |
julienrbrt
changed the title
DeleteFromHead method to StoreDeleteRange method to Store
julienrbrt
requested review from
tzdybal,
renaynay,
Wondertan,
vgonkivs,
gupadhyaya and
walldiss
as code owners
store/store_delete.go
Outdated
| deleteCtx := ctx | ||
| if deadline, ok := ctx.Deadline(); ok { | ||
| // allocate 95% of caller's set deadline for deletion | ||
| // and give leftover to save progress | ||
| sub := deadline.Sub(startTime) / 100 * 95 | ||
| var cancel context.CancelFunc | ||
| deleteCtx, cancel = context.WithDeadlineCause(ctx, startTime.Add(sub), errDeleteTimeout) | ||
| defer cancel() | ||
| } | ||
|
|
||
| if to-from < deleteRangeParallelThreshold { | ||
| height, missing, err = s.deleteSequential(deleteCtx, from, to) | ||
| } else { | ||
| height, missing, err = s.deleteParallel(deleteCtx, from, to) | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
Previously, updateTail was in the defer of the same function, so even if an error occurred, the partial progress was saved. This is no longer true as the call deleteRangeRaw short-circuits on error.
There was a problem hiding this comment.
should we return from this method the actual values that were successfully deleted to?
|
Thanks! Implementing your suggestions... |
This PR was blocked but got merged: #2638. It isn't bad to merge it, but we are relying on a fork of go-header for the apps until celestiaorg/go-header#347 is merged. Let's delete it in the main go.mod
|
Any update on this @Wondertan? |
|
We qim to get it done by this week |
| } | ||
|
|
||
| // Update HeadHeight if we deleted from the end | ||
| if to >= m.HeadHeight { |
There was a problem hiding this comment.
fail out here if to > m.HeadHeight (we should catch this err case in tests)
There was a problem hiding this comment.
Or actually do we have to allow this condition as we allow deletion --> beyond store head (to sync head) ?
There was a problem hiding this comment.
i think it is fine to be able to delete beyond. it allows you to put a big number if you do not know the head.
store/store_delete.go
Outdated
| deleteCtx := ctx | ||
| if deadline, ok := ctx.Deadline(); ok { | ||
| // allocate 95% of caller's set deadline for deletion | ||
| // and give leftover to save progress | ||
| sub := deadline.Sub(startTime) / 100 * 95 | ||
| var cancel context.CancelFunc | ||
| deleteCtx, cancel = context.WithDeadlineCause(ctx, startTime.Add(sub), errDeleteTimeout) | ||
| defer cancel() | ||
| } | ||
|
|
||
| if to-from < deleteRangeParallelThreshold { | ||
| height, missing, err = s.deleteSequential(deleteCtx, from, to) | ||
| } else { | ||
| height, missing, err = s.deleteParallel(deleteCtx, from, to) | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
should we return from this method the actual values that were successfully deleted to?
Co-authored-by: rene <[email protected]>
Co-authored-by: rene <[email protected]>
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. NOTE: PR titles should follow semantic commits: https://www.conventionalcommits.org/en/v1.0.0/ --> ## Overview With the fix from #2736, the `--skip-p2p-stores` flag isn't needed. Additionally, this bump go-header to the latest changes from celestiaorg/go-header#347 <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. Ex: Closes #<issue number> -->
Overview
d
Implement
DeleteRangeon store.This methods truncates the head to the given height. (different from DeleteTo that truncates the tail to the given height)