Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No snapshot after start in epoch #3623

Merged
merged 7 commits into from
Dec 9, 2021

Conversation

BeniaminDrasovean
Copy link
Contributor

In this PR: Add a specific key-value to each trie storer. When snapshoting, check if that value is present in the last storage, and only then start the snapshot. This way, after start in epoch no snapshot is triggered.

@codecov
Copy link

codecov bot commented Dec 6, 2021

Codecov Report

Merging #3623 (0bee4f6) into development (83ee059) will decrease coverage by 0.01%.
The diff coverage is 20.00%.

❗ Current head 0bee4f6 differs from pull request most recent head 5445cc8. Consider uploading reports for the commit 5445cc8 to get more accurate results
Impacted file tree graph

@@               Coverage Diff               @@
##           development    #3623      +/-   ##
===============================================
- Coverage        73.69%   73.67%   -0.02%     
===============================================
  Files              585      585              
  Lines            75317    75335      +18     
===============================================
+ Hits             55503    55506       +3     
- Misses           15405    15418      +13     
- Partials          4409     4411       +2     
Impacted Files Coverage Δ
trie/trieStorageManager.go 50.00% <6.66%> (-1.80%) ⬇️
state/accountsDB.go 75.67% <60.00%> (-0.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e0d4e9...5445cc8. Read the comment docs.


log.Debug("snapshotTrieStorageManager get",
"err", err.Error(),
"value", val,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the value nil? also, the code might panic because you don't check if the error is different than nil, so err.Error() would panic if err is nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored

func (tsm *trieStorageManager) EpochConfirmed(epoch uint32, _ uint64) {
tsm.flagDisableOldStorage.Toggle(epoch >= tsm.disableOldStorageEpoch)
log.Debug("old trie storage", "disabled", tsm.flagDisableOldStorage.IsSet())

err := tsm.mainStorer.Put([]byte(activeDBKey), []byte(activeDBVal))
log.LogIfError(err, "error", "set db as activeDB error")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better use

if err != nil {
    log.Error("error while putting active DB value into main storer", "error", err)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

func (tsm *trieStorageManager) EpochConfirmed(epoch uint32, _ uint64) {
tsm.flagDisableOldStorage.Toggle(epoch >= tsm.disableOldStorageEpoch)
log.Debug("old trie storage", "disabled", tsm.flagDisableOldStorage.IsSet())

err := tsm.mainStorer.Put([]byte(activeDBKey), []byte(activeDBVal))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like this is the only place when you set the value for active DB. I guess it is meant to be set only on the first confirmed epoch, right? So if starting from genesis, epoch 0 -> epoch 1 will trigger the value to be set; or when starting in epoch, only the next epoch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This func is called every time when an epoch is changed, or when the trieStorageManager is initialized, so activeDBKey will be saved in each storer (each epoch a new storer is created)

@@ -493,6 +502,19 @@ func (tsm *trieStorageManager) takeCheckpoint(checkpointEntry *snapshotsQueueEnt
}
}

func isActiveDb(stsm *snapshotTrieStorageManager) bool {
val, err := stsm.Get([]byte(activeDBKey))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check the err for nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

bogdan-rosianu
bogdan-rosianu previously approved these changes Dec 7, 2021
SebastianMarian
SebastianMarian previously approved these changes Dec 7, 2021
Copy link
Collaborator

@gabi-vuls gabi-vuls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System test passed.
image

@BeniaminDrasovean BeniaminDrasovean merged commit cc1df47 into development Dec 9, 2021
@BeniaminDrasovean BeniaminDrasovean deleted the no-snapshot-on-startInEpoch branch December 9, 2021 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants