[MemIAVL] Create snapshot whenever height diff exceed interval#109
[MemIAVL] Create snapshot whenever height diff exceed interval#109
Conversation
| time.Sleep(10 * time.Millisecond) | ||
| require.Nil(t, db.snapshotRewriteChan, "rewrite should not start at height %d", i) | ||
| // snapshot version should remain 0 until rewrite | ||
| require.Equal(t, int64(0), db.MultiTree.SnapshotVersion()) |
There was a problem hiding this comment.
You can use EqualValues to avoid casting to int64. Ditto in other places.
| require.NoError(t, err) | ||
|
|
||
| // helper to commit one change to bump height | ||
| commitOnce := func(key, val string) int64 { |
There was a problem hiding this comment.
Generally I recommend refactoring this type of function into one that laso takes t, with its name prefixed with require, e.g. requireCommitOnce(t *testing.T, key,val string), then call t.Helper() in its first line.
That then makes up an executable way to document intent and would play nice with the go testing toolkit.
| // After completion, snapshot version should be 6 | ||
| require.Equal(t, int64(6), db.MultiTree.SnapshotVersion()) | ||
|
|
||
| require.NoError(t, db.Close()) |
There was a problem hiding this comment.
Move this into t.Cleanup to make sure it's executed after test ends (whether it fails or not).
| import ( | ||
| "context" | ||
| "errors" | ||
| "os" |
There was a problem hiding this comment.
The check failures here are genuine. Please clean up imports.
| if err := db.rewriteSnapshotBackground(); err != nil { | ||
| db.logger.Error("failed to rewrite snapshot in background", "err", err) | ||
| // create snapshot when current height - last snapshot height > interval | ||
| if db.snapshotInterval > 0 && height-db.MultiTree.SnapshotVersion() > int64(db.snapshotInterval) { |
There was a problem hiding this comment.
Thinking out loud, do we need to sanity check the height? For example could a bad hight result in a snapshot rewrite by accident? Might be worth clamping the range of height etc.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (69.23%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #109 +/- ##
==========================================
+ Coverage 64.03% 64.08% +0.05%
==========================================
Files 28 28
Lines 4048 4057 +9
==========================================
+ Hits 2592 2600 +8
Misses 1161 1161
- Partials 295 296 +1
🚀 New features to boost your workflow:
|
3 participants
Describe your changes and provide context
Problem:
Currently, if snapshot interval is set to 10k, and previous snapshot is created at height 10000, when node got restart at height 20001, it will not start the next snapshot creation until height 30000. This will actually lead to higher snapshot gaps which will then lead to higher restart time.
Instead of using fixed window interval, we actually switched to a new snapshot policy where the next snapshot creation will happen immediately at current height after commit as long as the current height - last snapshot height >= snapshot interval.
Testing performed to validate your change
Added unit test + Tested on RPC node