fix(store): improve pruning to avoid skipped heights #1058
Conversation
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
block/pruning.go
Outdated
| if retainHeight <= m.State.BaseHeight { | ||
| return 0, nil | ||
| } |
block/pruning.go
Outdated
| if m.IsProposer() && m.NextHeightToSubmit() < retainHeight { // do not delete anything that we might submit in future | ||
| return fmt.Errorf("cannot prune blocks before they have been submitted: retain height %d: next height to submit: %d: %w", | ||
| retainHeight, | ||
| m.NextHeightToSubmit(), | ||
| gerrc.ErrInvalidArgument) | ||
| m.logger.Debug("cannot prune blocks before they have been submitted. using height last submitted height for pruning", "retain_height", retainHeight, "height_to_submit", m.NextHeightToSubmit()) | ||
| retainHeight = m.NextHeightToSubmit() - 1 |
There was a problem hiding this comment.
I think there is a race here because you read m.NextHeightToSubmit() twice it can change in between
There was a problem hiding this comment.
also isnt this off by one? why isn't retain height just m.NextHeightToSubmit(?
| ) | ||
|
|
||
| func (m *Manager) PruneBlocks(retainHeight uint64) error { | ||
| func (m *Manager) PruneBlocks(retainHeight uint64) (uint64, error) { |
There was a problem hiding this comment.
can you document return value
block/pruning_test.go
Outdated
| expectedPruned := uint64(0) | ||
| if lastSubmitted >= manager.State.BaseHeight { | ||
| expectedPruned = lastSubmitted - manager.State.BaseHeight | ||
| } |
There was a problem hiding this comment.
can be 1 liner with math.max
| if err != nil { | ||
| return fmt.Errorf("load block id from store %d: %w", h, err) | ||
| c.logger.Error("load blocksync block id from store", "height", h, "error", err) | ||
| continue |
There was a problem hiding this comment.
there is no cid stored for a specific height, which may happen
why may it happen?
There was a problem hiding this comment.
I guess it can happen a lot right, since blocks maybe came from another source 👍
There was a problem hiding this comment.
do you even need to log error then? probably want to differentiate not found vs parse error
store/pruning.go
Outdated
| // PruneBlocks removes block up to (but not including) a height. It returns number of blocks pruned. | ||
| func (s *DefaultStore) PruneBlocks(from, to uint64) (uint64, error) { | ||
| func (s *DefaultStore) PruneStore(from, to uint64, logger types.Logger) (uint64, error) { |
store/pruning.go
Outdated
| if err := batch.Delete(getCidKey(height)); err != nil { | ||
| return err | ||
| } | ||
| return nil |
store/pruning.go
Outdated
| if err := batch.Delete(getSequencersKey(height)); err != nil { | ||
| return err | ||
| } | ||
| return nil |
store/pruning.go
Outdated
| if err := batch.Delete(getResponsesKey(height)); err != nil { | ||
| return err | ||
| } | ||
| return nil |
store/pruning.go
Outdated
| if err != nil { | ||
| continue | ||
| } |
srene
force-pushed
the
srene/1039-pruning-issues-fix
branch
from
eed5605 to
c51bce1
Compare
PR Standards
Opening a pull request should be able to meet the following requirements
In case there is some height with a key missing in the store, or in case pruning fails at a specific height for some reason, the pruning returns error.
In this PR this logic is changed and the pruning continues for other heights.
PR naming convention: https://hackmd.io/@nZpxHZ0CT7O5ngTp0TP9mg/HJP_jrm7A
Close #1039
<-- Briefly describe the content of this pull request -->
For Author:
godoccommentsFor Reviewer:
After reviewer approval: