Skip to content

Comments

[Persistence] Adds atomic Update for TreeStore#861

Merged
dylanlott merged 34 commits intomainfrom
persistence/savepoints-initial
Jul 31, 2023
Merged

[Persistence] Adds atomic Update for TreeStore#861
dylanlott merged 34 commits intomainfrom
persistence/savepoints-initial

Conversation

@dylanlott
Copy link
Contributor

@dylanlott dylanlott commented Jun 27, 2023 •
edited
Loading

Description

Adds atomic savepoint and rollback creation to the TreeStore

Issue

Relates to #562

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 savepoints and rollbacks to TreeStore Update function

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 Jun 27, 2023
@dylanlott dylanlott self-assigned this Jun 27, 2023
@dylanlott dylanlott added persistence Persistence specific changes code health Nice to have code improvement labels Jun 27, 2023
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch from 3f68ff0 to 2d9022f Compare June 27, 2023 23:32
h5law
h5law reviewed Jun 28, 2023
View reviewed changes
@dylanlott dylanlott mentioned this pull request Jun 29, 2023
Closed
20 tasks
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch from 7ee9be3 to 6030ada Compare June 29, 2023 18:14
This was referenced Jun 29, 2023
Closed
Merged
Open
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch from 6030ada to a7ba81a Compare July 6, 2023 00:05
h5law
h5law reviewed Jul 6, 2023
View reviewed changes
h5law
h5law reviewed Jul 6, 2023
View reviewed changes
h5law
h5law reviewed Jul 6, 2023
View reviewed changes
persistence/trees/trees_test.go Outdated
}

// createMockBus returns a mock bus with stubbed out functions for bus registration
func createMockBus(t *testing.T, runtimeMgr modules.RuntimeMgr) *mockModules.MockBus {
Copy link
Contributor

@h5law h5law Jul 6, 2023

Choose a reason for hiding this comment

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

NIT: I would consider moving all these helpers to a file named trees_utils_test.go

Copy link
Collaborator

@Olshansk Olshansk Jul 28, 2023

Choose a reason for hiding this comment

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

@dylanlott Thoughts on this?

Copy link
Contributor Author

@dylanlott dylanlott Jul 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rather create an issue with a TECHDEBT comment. There's no easy answer to this without answering larger questions around import cycles and how modules should be imported.

h5law
h5law reviewed Jul 6, 2023
View reviewed changes
h5law
h5law reviewed Jul 6, 2023
View reviewed changes
h5law
h5law reviewed Jul 6, 2023
View reviewed changes
h5law
h5law reviewed Jul 6, 2023
View reviewed changes
h5law
h5law reviewed Jul 6, 2023
View reviewed changes
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch 2 times, most recently from 26b1e44 to ef4ed4d Compare July 11, 2023 21:03
@dylanlott dylanlott changed the base branch from main to persistence/tree-store-submodule July 11, 2023 21:05
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch from ef4ed4d to be56f6a Compare July 11, 2023 21:06
@dylanlott dylanlott force-pushed the persistence/tree-store-submodule branch from 3741a01 to 7165f7c Compare July 11, 2023 21:22
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch from be56f6a to f0c6203 Compare July 11, 2023 21:24
@dylanlott dylanlott force-pushed the persistence/tree-store-submodule branch from 7165f7c to 6181db1 Compare July 12, 2023 14:56
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch 2 times, most recently from 1cfec18 to 2d53dc1 Compare July 12, 2023 19:54
dylanlott added 17 commits July 27, 2023 10:30
@dylanlott
[fixup] fix path for treestore mock generations
7f947f9
@dylanlott
[fixup] use method to fetch context instead
d70a067
@dylanlott
[fixup] adds comments and updates tests
d107ed0
@dylanlott
[fixup] adds optimize comment to save function
626ea5d
@dylanlott
[fixup] cleanup
b3ab6b0
@dylanlott
[docs] updates package level comment
fc04ab0
@dylanlott
[fixup] adds comments and updates logger use
2af89dd
@dylanlott
8ed31bf
@dylanlott
[rename] s/NewSavePoint/SetSavePoint/g
09097ef
@dylanlott
[fixup] update test names and use constants in tests
07b64da
@dylanlott
[fixup] adds better comment to treeStore#Rollback
1ecfde6
@dylanlott
[tests] adds a test for failing rollback
a687768
@dylanlott
[fixup] check rollback error value to satisfy linter
800b2cb
@dylanlott
[test] adds a test for block proposal revert
0b22519
@dylanlott
[fixup] fix linter error
ba85105
@dylanlott
[fixup] updates rollback test to use Errors#Is
7a7d95c
@dylanlott
[fixup] prevent import cycle error errors in treestore
0f87f0b
@dylanlott dylanlott force-pushed the persistence/savepoints-initial branch from 29ad7df to 0f87f0b Compare July 27, 2023 17:24
@dylanlott dylanlott requested a review from Olshansk July 27, 2023 18:49
@dylanlott dylanlott mentioned this pull request Jul 27, 2023
Closed
20 tasks
Olshansk
Olshansk requested changes Jul 28, 2023
View reviewed changes
Copy link
Collaborator

@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 last minute nits and then it should be g2g.

@dylanlott Can you please merge with main and resolve conflicts to avoid a force push? It allows the reviewer to review changes since last review. Ty!

persistence/trees/main_test.go
Comment on lines +1 to +3
//go:build test

package trees
Copy link
Collaborator

@Olshansk Olshansk Jul 28, 2023

Choose a reason for hiding this comment

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

In persistence/trees/trees_test.go, we use package trees_test and not have the //go:build test tag.

Here, we have the package trees and do have the //go:build test tag.

Feels inconsistent, but not sure if this is general best practice for main_test.go` vs non-main test files?

Copy link
Contributor Author

@dylanlott dylanlott Jul 28, 2023

Choose a reason for hiding this comment

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

I was following the prescription of TestUtils for testing unexported members like previously discussed.

persistence/trees/trees_test.go
}
}

// TODO_AFTER(#861): Implement this test with the test suite available in #861
Copy link
Collaborator

@Olshansk Olshansk Jul 28, 2023

Choose a reason for hiding this comment

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

test suite available in #861 is this correct?

We want to use the test suite in this PR to implement it?

Copy link
Contributor Author

@dylanlott dylanlott Jul 28, 2023

Choose a reason for hiding this comment

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

I believe that's what @h5law meant out of this, yes.

persistence/trees/trees_test.go
nodeStore: nodeStore,
}
// TECHDEBT(#796): Test helpers should be consolidated in a single place
func newTestApp(t *testing.T) *core_types.Actor {
Copy link
Collaborator

@Olshansk Olshansk Jul 28, 2023

Choose a reason for hiding this comment

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

OPTIONAL: Per @h5law's comment, we could move the helpers into a separate utils_test.go but don't feel strongly about it.

@dylanlott dylanlott requested a review from Olshansk July 29, 2023 01:33
@dylanlott dylanlott merged commit fe12410 into main Jul 31, 2023
@dylanlott dylanlott deleted the persistence/savepoints-initial branch July 31, 2023 16:04
red-0ne pushed a commit that referenced this pull request Aug 2, 2023
@dylanlott @Olshansk @red-0ne
[Persistence] Adds atomic Update for TreeStore (#861)
298b08f 
Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Olshansk Olshansk Olshansk approved these changes

+1 more reviewer

@h5law h5law h5law approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dylanlott dylanlott

Labels

code health Nice to have code improvement 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

M1: Pocket PoS (Proof of Stake)

Development

Successfully merging this pull request may close these issues.

3 participants

@dylanlott @Olshansk @h5law