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

feat: ADR 040: Implement in-memory DB backend #9952

Merged
merged 12 commits into from
Aug 31, 2021

Conversation

roysc
Copy link
Contributor

@roysc roysc commented Aug 17, 2021

Description

Implements an in-memory backend for the DB interface introduced by #9573 and specified by ADR-040. This expands on the btree-based MemDB from tm-db by using copy-on-write clones to implement versioning.

Resolves: vulcanize#2

Will move out of draft once #9573 is merged and rebased on.

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules n/a
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@roysc roysc force-pushed the roysc/adr-040-db-memdb branch 3 times, most recently from a80c49d to f189a95 Compare August 18, 2021 11:36
@roysc roysc changed the title feat: ADR 040: Implement in-memory DB feat: ADR 040: Implement in-memory DB backend Aug 18, 2021
@roysc roysc force-pushed the roysc/adr-040-db-memdb branch 2 times, most recently from 9bdcfa2 to 15501e8 Compare August 22, 2021 01:02
@roysc roysc marked this pull request as ready for review August 22, 2021 01:03
@roysc roysc marked this pull request as draft August 22, 2021 06:47
@roysc roysc force-pushed the roysc/adr-040-db-memdb branch from 3867290 to 1358c4f Compare August 23, 2021 03:09
@roysc roysc force-pushed the roysc/adr-040-db-memdb branch from 1358c4f to c8cd01d Compare August 23, 2021 03:50
@roysc roysc marked this pull request as ready for review August 23, 2021 05:05
redundant since dbWriter uses a clone
@roysc roysc force-pushed the roysc/adr-040-db-memdb branch from 3638b40 to e17792b Compare August 23, 2021 07:21
@roysc
Copy link
Contributor Author

roysc commented Aug 23, 2021

Now that the interface has been merged, this is good to review. This contains MemDB, unit tests, and supporting utilities, which logically fit together but make a large PR, so let me know if it would be better to break the code down further.

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.

Changes mainly LGTM, great work. Left some small remarks 👍

CHANGELOG.md Show resolved Hide resolved
db/README.md Outdated Show resolved Hide resolved
db/README.md Show resolved Hide resolved
db/memdb/db.go Outdated Show resolved Hide resolved
func (db *MemDB) Versions() (dbm.VersionSet, error) {
db.mtx.RLock()
defer db.mtx.RUnlock()
return db.vmgr, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

do we not want to return a copy of the map?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we already have a Copy method too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted to make this copy-on-write. Versions is called whenever version history is queried, and if the history becomes large, it could be expensive. So a vmgr is just copied over whenever versions are saved/deleted, and the caller can safely keep the old reference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

vmgr is a pointer, so we don't make any object copy here

db/types.go Outdated Show resolved Hide resolved
return &vmIterator{ch: ch}
}

// Count implements VersionSet.
Copy link
Contributor

Choose a reason for hiding this comment

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

If this type implements an interface, we don't need to add godocs for the sake of adding godocs 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine with me, I was just following the existing practice


// Error implements Iterator.
func (i *memDBIterator) Error() error {
return nil // famous last words
Copy link
Contributor

Choose a reason for hiding this comment

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

lol, any reason not to track an error? Or can one never happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there's no error case for this iterator - it's only invalidated when it reaches its end.

Copy link
Contributor

Choose a reason for hiding this comment

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

ACK. Let's remove // famous last words then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha sure, that was from the tm-db code.

roysc and others added 2 commits August 24, 2021 13:53
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #9952 (3494b38) into master (54b0556) will increase coverage by 0.24%.
The diff coverage is n/a.

❗ Current head 3494b38 differs from pull request most recent head b56dd6e. Consider uploading reports for the commit b56dd6e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9952      +/-   ##
==========================================
+ Coverage   63.56%   63.80%   +0.24%     
==========================================
  Files         572      566       -6     
  Lines       53584    53363     -221     
==========================================
- Hits        34058    34047      -11     
+ Misses      17583    17397     -186     
+ Partials     1943     1919      -24     
Impacted Files Coverage Δ
server/mock/tx.go 63.15% <0.00%> (-1.85%) ⬇️
x/auth/keeper/grpc_query.go 57.14% <0.00%> (-0.86%) ⬇️
types/address.go 68.69% <0.00%> (ø)
x/auth/vesting/types/msgs.go 0.00% <0.00%> (ø)
x/auth/middleware/index_events.go
x/auth/middleware/run_msgs.go
x/auth/middleware/recovery.go
x/auth/middleware/middleware.go
x/auth/middleware/legacy_router.go
x/auth/middleware/gas.go
... and 18 more

@robert-zaremba robert-zaremba mentioned this pull request Aug 24, 2021
4 tasks
Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

@robert-zaremba want to review this so we can merge?

@alexanderbez
Copy link
Contributor

Let's give @robert-zaremba a day to review. Is that okay?

@tac0turtle
Copy link
Member

Let's give @robert-zaremba a day to review. Is that okay?

haha, more than one day is fine as well. It was mainly a ping, not a "needs to be done now"

@tac0turtle tac0turtle added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 31, 2021
@tac0turtle
Copy link
Member

@roysc once the PR is updated to master the bot will merge it

@mergify mergify bot merged commit 2c31451 into cosmos:master Aug 31, 2021
@orijbot
Copy link

orijbot commented Aug 31, 2021

@roysc roysc deleted the roysc/adr-040-db-memdb branch August 31, 2021 10:33
@odeke-em
Copy link
Collaborator

odeke-em commented Oct 7, 2021

Surprisingly the code from this change is failing tests locally

$ go test
# github.com/cosmos/cosmos-sdk/db/memdb [github.com/cosmos/cosmos-sdk/db/memdb.test]
./db.go:44:23: incompatible type: cannot use (*MemDB)(nil) (value of type *MemDB) as db.DBConnection value
./db_test.go:32:9: incompatible type: cannot use NewDB() (value of type *MemDB) as db.DBConnection value
FAIL	github.com/cosmos/cosmos-sdk/db/memdb [build failed]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:Store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Epic 2: Replace tm-db database interface with new cosmos-db
6 participants