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

R4R: Store Refactor #2344

Closed
wants to merge 22 commits into from
Closed

R4R: Store Refactor #2344

wants to merge 22 commits into from

Conversation

mossid
Copy link
Contributor

@mossid mossid commented Sep 15, 2018 •
edited
Loading

Closes: #2309

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer

  • Each store has moved to their own folder
  • Store related files, such as gas.go, store.go, are moved to store/
  • types/ imports store/ now and reexports types/functions
  • Remove StoreType enum, instead StoreKeys are dependent on concrete store implementation
  • Made some code duplication at errors.go to reduce dependency
  • traceWriter io.Writer and traceContext TraceContext are merged into Tracer
  • meter GasMeter and config GasConfig are merged into GasTank
  • Store interfaces are refactored:
            KVStore         CommitStore          MultiStore              ^
           /        \         /        \           /       \             |
         /            \     /            \       /          \            |  embeds
CacheKVStore    CommitKVStore    CommitMultiStore    CacheMultiStore     *

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@mossid mossid added the wip label Sep 15, 2018
@codecov
Copy link

codecov bot commented Sep 16, 2018 •
edited
Loading

Codecov Report

Merging #2344 into develop will decrease coverage by 0.3%.
The diff coverage is 70.77%.

@@             Coverage Diff             @@
##           develop    #2344      +/-   ##
===========================================
- Coverage    58.82%   58.51%   -0.31%     
===========================================
  Files          152      144       -8     
  Lines         9425     8213    -1212     
===========================================
- Hits          5544     4806     -738     
+ Misses        3511     3091     -420     
+ Partials       370      316      -54

@cwgoes cwgoes self-assigned this Oct 3, 2018
@mossid mossid changed the title WIP: Store Refactor R4R: Store Refactor Oct 3, 2018
@mossid mossid force-pushed the joon/2309-store-refactor branch from a43b50f to 80a4921 Compare October 6, 2018 16:55
@mossid mossid changed the title R4R: Store Refactor WIP: Store Refactor Oct 7, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Oct 25, 2018 •
edited
Loading

@mossid Do you want to rebase this? I still think the refactor is a great idea and would be happy to review (sorry it didn't happen sooner)!

@mossid mossid force-pushed the joon/2309-store-refactor branch from db4905a to ab13269 Compare October 26, 2018 10:43
@cwgoes cwgoes mentioned this pull request Oct 28, 2018
Merged
5 tasks
@mossid mossid requested a review from zramsay as a code owner October 29, 2018 17:19
@mossid mossid force-pushed the joon/2309-store-refactor branch from 218f495 to bc2b7f7 Compare October 29, 2018 18:16
@sunnya97 sunnya97 changed the title WIP: Store Refactor R4R: Store Refactor Oct 30, 2018
@cwgoes
Copy link
Contributor

cwgoes commented Nov 1, 2018

@mossid Thanks for rebasing. Let's pair review this in person, maybe early next week.

@mossid mossid force-pushed the joon/2309-store-refactor branch from 483ecfa to e1bd6dd Compare November 1, 2018 14:42
alexanderbez
alexanderbez reviewed Nov 1, 2018
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

@mossid this PR is pretty hefty...I really don't know how to start as I'm sure I'll miss something. May I suggest perhaps breaking it up into sub PR (I, II, III, etc...) if possible?

Thoughts @ValarDragon?

baseapp/baseapp.go
@@ -115,7 +115,7 @@ func (app *BaseApp) Name() string {
// SetCommitMultiStoreTracer sets the store tracer on the BaseApp's underlying
// CommitMultiStore.
func (app *BaseApp) SetCommitMultiStoreTracer(w io.Writer) {
app.cms.WithTracer(w)
app.cms.GetTracer().Writer = w
Copy link
Contributor

Choose a reason for hiding this comment

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

The initial API call was bad and that's my fault. Can we just do app.cms.SetTracer(w)?

cmd/gaia/app/app.go
keyMain *sdk.KVStoreKey
keyAccount *sdk.KVStoreKey
keyStake *sdk.KVStoreKey
keyMain *sdk.IAVLStoreKey
Copy link
Contributor

Choose a reason for hiding this comment

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

Mhmmm, why did we rename these? This assumes IAVL usage, although it probably won't change for Gaia.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want us to have this key... an IAVLStore is a KVStore... no need to create key types for specific types of KVStores. In the future we may have other store types like HeapStores.

@fedekunze
Copy link
Collaborator

lol this PR is gigantic

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Nov 29, 2018 •
edited
Loading

@mossid way out of date - please update, we should get this in

@cwgoes cwgoes mentioned this pull request Nov 30, 2018
Closed
@mossid mossid mentioned this pull request Dec 3, 2018
Closed
@mossid
Copy link
Contributor Author

mossid commented Dec 3, 2018

Closing in favor of #2986

@mossid mossid closed this Dec 3, 2018
@mossid mossid deleted the joon/2309-store-refactor branch December 3, 2018 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexanderbez alexanderbez alexanderbez left review comments

@jaekwon jaekwon jaekwon left review comments

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@zramsay zramsay Awaiting requested review from zramsay

Assignees

@cwgoes cwgoes

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mossid @cwgoes @fedekunze @rigelrozanski @jaekwon @alexanderbez @sunnya97