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.

Sign up for GitHub

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

Jump to bottom

[Persistence] Adds Save and Load functionality to TreeStore #897

Closed
wants to merge 10 commits into from
Closed

[Persistence] Adds Save and Load functionality to TreeStore #897

wants to merge 10 commits into from

Conversation

dylanlott
Copy link
Contributor

@dylanlott dylanlott commented Jul 11, 2023 •
edited
Loading

Description

Adds save and load functionality to the TreeStore.

This PR makes the TreeStore an Interruptable module. This is necessary because the Persistence module needs to be able to tell the TreeStore to relinquish its connection to the nodeStores which are backed by BadgerDB.

Issue

Related to #568

Type of change

Please mark the relevant option(s):

  • New feature, functionality or library
  • Bug fix
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other

List of changes

  • Adds save and load functionality for loading a worldState from file directories.

Testing

  • make develop_test; if any code changes were made
  • make test_e2e on k8s LocalNet; if any code changes were made
  • e2e-devnet-test passes tests on DevNet; if any code was changed
  • Docker Compose LocalNet; if any major functionality was changed or introduced
  • k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
  • I have tested my changes using the available tooling
  • I have updated the corresponding CHANGELOG

If Applicable Checklist

  • I have updated the corresponding README(s); local and/or global
  • I have added tests that prove my fix is effective or that my feature works
  • I have added, or updated, mermaid.js diagrams in the corresponding README(s)
  • I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

@reviewpad reviewpad bot added the large Pull request is large label Jul 11, 2023
@dylanlott dylanlott self-assigned this Jul 11, 2023
@dylanlott dylanlott added the persistence Persistence specific changes label Jul 11, 2023
@dylanlott dylanlott changed the base branch from main to persistence/savepoints-initial July 11, 2023 21:45
@reviewpad reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Jul 11, 2023
@dylanlott dylanlott changed the title Adds Save and Load functionality to TreeStore [Persistence] Adds Save and Load functionality to TreeStore Jul 11, 2023
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch from 0a6c72f to 7de2528 Compare July 12, 2023 15:12
@dylanlott dylanlott changed the title [Persistence] Adds Save and Load functionality to TreeStore [WIP][Persistence] Adds Save and Load functionality to TreeStore Jul 12, 2023
@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jul 12, 2023
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch 2 times, most recently from 2d53dc1 to 15d5c8a Compare July 17, 2023 19:15
@reviewpad reviewpad bot added medium Pull request is medium and removed large Pull request is large labels Jul 17, 2023
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch from 15d5c8a to a6fe96f Compare July 18, 2023 20:37
@dylanlott dylanlott mentioned this pull request Jul 19, 2023
Merged
20 tasks
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch from 788dbbe to c9caaa3 Compare July 20, 2023 00:27
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch 2 times, most recently from 9de1b01 to dc4f205 Compare July 20, 2023 17:08
@reviewpad reviewpad bot added large Pull request is large and removed medium Pull request is medium labels Jul 20, 2023
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch 2 times, most recently from 5b7af11 to a1e0a10 Compare July 24, 2023 18:15
@dylanlott dylanlott force-pushed the persistence/save-load branch 2 times, most recently from 8b4844c to 4765b3a Compare July 26, 2023 17:04
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch 2 times, most recently from 29ad7df to 0f87f0b Compare July 27, 2023 17:24
Base automatically changed from persistence/savepoints-initial to main July 31, 2023 16:04
@dylanlott
[WIP] initial commit gets save and load working
0d34af1
@dylanlott
[wip] adds start and stop implementation
aba7feb 
* fulfills the Interruptable module interface to allow for control over
  the node stores lifecycle to enable save and load functionality
* adds tests for the Start and Stop functionality
* adds nil checks to GetTree function to make testing start and stop
  behavior predictable
@dylanlott
[wip] gets destination backup working for KVStore
2c6fb5c
@dylanlott dylanlott changed the title [WIP][Persistence] Adds Save and Load functionality to TreeStore [Persistence] Adds Save and Load functionality to TreeStore Aug 1, 2023
@dylanlott dylanlott added e2e-devnet-test Runs E2E tests on devnet waiting-for-review labels Aug 1, 2023
@dylanlott dylanlott marked this pull request as ready for review August 1, 2023 21:27
Olshansk
Olshansk requested changes Aug 2, 2023
View reviewed changes
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

A few comments throughout but nothing "glaring" or blocking.

Thanks for getting this moving Dylan!

persistence/blockstore/block_store.go Show resolved Hide resolved
persistence/kvstore/kvstore.go Show resolved Hide resolved
persistence/trees/trees_test.go Show resolved Hide resolved
persistence/kvstore/kvstore_test.go Outdated Show resolved Hide resolved
persistence/kvstore/kvstore_test.go
func setupStore(t *testing.T, store KVStore) {
t.Helper()
err := store.Set([]byte("foo"), []byte("bar"))
require.NoError(t, err)
err = store.Set([]byte("baz"), []byte("bin"))
require.NoError(t, err)
}

func isEmpty(t *testing.T, dir string) (bool, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not go with an approach like this: https://stackoverflow.com/a/24102536/768439 ?

Seems shorter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Beacuse the linter complains if ioutil is used since it's officially deprecated.

Copy link
Contributor

@h5law h5law Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

why not use

dirEntries, err := os.ReadDir(dir)
require.NoError(t, err)
if len(dirEntries) == 0 {
	return true
}
return false

https://pkg.go.dev/os#ReadDir

This is what ioutil docs point to as the newer more efficient approach to the deprecated method

persistence/trees/trees.go Outdated Show resolved Hide resolved
persistence/trees/trees.go Outdated Show resolved Hide resolved
persistence/trees/trees.go Outdated Show resolved Hide resolved
persistence/trees/trees.go Outdated Show resolved Hide resolved
persistence/trees/trees.go Outdated Show resolved Hide resolved
@Olshansk
Copy link
Member

Olshansk commented Aug 4, 2023

@dylanlott Can you "re-request" a review when its ready and I'll reply to all the comments/questions then?

@dylanlott
Copy link
Contributor Author

@dylanlott Can you "re-request" a review when its ready and I'll reply to all the comments/questions then?

You beat me to it. I just finished up addressing your feedback, it's ready for re-review 👍

@dylanlott dylanlott requested a review from Olshansk August 4, 2023 21:03
@h5law h5law removed their request for review December 10, 2023 20:38
@dylanlott dylanlott closed this Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h5law h5law h5law left review comments

@Olshansk Olshansk Awaiting requested review from Olshansk

Assignees

@dylanlott dylanlott

Labels
e2e-devnet-test Runs E2E tests on devnet large Pull request is large persistence Persistence specific changes waiting-for-review
Projects
V1 Dashboard
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dylanlott @Olshansk @h5law