feat: Pruning and compaction port to 0.38#2
Conversation
| metrics *Metrics | ||
|
|
||
| // Preserve the number of state entries pruned. | ||
| // Used to calculated correctly when to trigger compactions |
There was a problem hiding this comment.
| // Used to calculated correctly when to trigger compactions | |
| // Used to calculate correctly when to trigger compactions |
state/store.go
Outdated
|
|
||
| return nil | ||
| // We do not want to panic or interrupt consensus on compaction failure | ||
| if store.StoreOptions.Compact && previosulyPrunedStates+pruned >= uint64(store.StoreOptions.CompactionInterval) { |
There was a problem hiding this comment.
I think there's something here that doesn't add up.
previosulyPrunedStatesis passed as a parameter- There's only one callsite, and
previosulyPrunedStatesis assigned fieldprunedStatesof the pruner - So, successive calls to this method (
PruneStates) will correctly accumulate the number of pruned states over time - HOWEVER, when we reach
CompactionInterval, and this if is hit, we will call compact, but fieldprunedStatesof the pruner is not reset - So... once
CompactionIntervalis reached this IF will be hit every time... callingstore.db.Compact(nil, nil)in every call to this method
... or am I missing something?
There was a problem hiding this comment.
Yes, this has to be reset for this condition to work. And the condition for ABCI results should follow the logic of this. Will update this and ping you
state/store.go
Outdated
| } | ||
|
|
||
| if forceCompact && store.Compact { | ||
| if pruned+batchPruned >= store.CompactionInterval || targetRetainHeight-lastRetainHeight >= store.CompactionInterval { |
There was a problem hiding this comment.
I also have a problem with this condition. If I understood the code well
- if condition
pruned+batchPruned >= store.CompactionIntervalis true, - then condition
targetRetainHeight-lastRetainHeightis also true
... because pruned+batchPruned will increase by 1 at every iteration of the loop.
Since both conditions are combined with a logical OR, then the second condition is redundant.
Thinking more generally, why do we care about targetRetainHeight and lastRetainHeight here? Shouldn't we just care about the first condition, i.e., the actual number of ABCI responses pruned?
There was a problem hiding this comment.
Another comment here, if we pruned some ABCIResponses but less than store.CompactionInterval, we won't be compacting, but:
- How do we keep track of the cumulative number of ABCIResponses pruned but not compacted?
- Imagine a node that calls
PruneABCIResponses1000 times, but each time it prunes less thanstore.CompactionInterval. After the 1000 calls, no compaction will be done IIUC -- is that what we want?
There was a problem hiding this comment.
I think targetRetainHeight-lastRetainHeight was there to guard against your second comment. But thinking about it, it is not actually doing that. It is simply indicating there was A change (that is also why the compaction will work because it will compact every time), but what we want is a cumulative diff. So this needs to be fixed.
state/store.go
Outdated
| store.StoreOptions.ResultsToCompact += uint64(pruned + batchPruned) | ||
| if forceCompact && store.Compact && store.StoreOptions.ResultsToCompact >= (uint64)(store.StoreOptions.CompactionInterval) { |
There was a problem hiding this comment.
I would structure this differently. If store.Compact is false, we don't want store.StoreOptions.ResultsToCompact to be growing forever and eventually wrapping.
I would add an outer IF with checking store.Compact only. And, if true, then we update store.StoreOptions.ResultsToCompact and do the inner IF with the rest of the conditions.
state/pruner.go
Outdated
| prunedStates, err := p.stateStore.PruneStates(base, height, evRetainHeight, p.prunedStates) | ||
| p.prunedStates += prunedStates | ||
|
|
||
| p.prunedStates, err = p.stateStore.PruneStates(base, height, evRetainHeight, p.prunedStates) |
There was a problem hiding this comment.
Is p.prunedStates now used for anything?
There was a problem hiding this comment.
Also, the last parameter in PruneStates is unused within the function
This is a manual back port of https://github.com/cometbft/cometbft/pull/2356/files#diff-fe3f53c9e61cbc76a2639c55769478a908fe350e272b241250b50489ebc38568 . I copied the changes to not include indexer related things as the indexer pruning is not existing yet in the target branch.
PR checklist
.changelog(we use unclog to manage our changelog)docs/orspec/) and code comments