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

Naive DB + Rocksdb implemenation #125

Merged
merged 23 commits into from
Jan 6, 2020
Merged

Naive DB + Rocksdb implemenation #125

merged 23 commits into from
Jan 6, 2020

Conversation

GregTheGreek
Copy link
Member

@GregTheGreek GregTheGreek commented Dec 26, 2019

This pr introduces a naive implementation of the database traits (read & write), and an implementation of rocksdb. Something to note about database implementations in rust is that they're built with lifetimes, thus they will always close, there is no need to gracefully shut them down.

Discussion points:

  • DatabaseService trait is currently commented out since I ran into sizing issues return Self open to discussions on how to solve that. The implementation has Open()
  • Research on leveldb showed that its fairly unmaintained and using old Rust idioms.
  • How do we handle reads that return no result? Check out RocksDb::read or RocksDb::bulk_read

Closes #110

Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

:D would be nice to have an in memory db for testing purposes, so I will open an issue to add this after (I can do this as I know how it can be done, but could leave if someone else wants to do)

node/db/src/errors.rs Outdated Show resolved Hide resolved
node/db/src/errors.rs Outdated Show resolved Hide resolved
node/db/src/traits.rs Outdated Show resolved Hide resolved
node/db/tests/db_utils/mod.rs Outdated Show resolved Hide resolved
node/db/tests/db_utils/mod.rs Show resolved Hide resolved
node/db/src/rocks.rs Outdated Show resolved Hide resolved
node/db/src/rocks.rs Outdated Show resolved Hide resolved
node/db/src/rocks.rs Outdated Show resolved Hide resolved
node/db/src/rocks.rs Outdated Show resolved Hide resolved
node/db/src/rocks.rs Outdated Show resolved Hide resolved
node/db/src/traits.rs Outdated Show resolved Hide resolved
node/db/src/errors.rs Outdated Show resolved Hide resolved
node/db/src/lib.rs Outdated Show resolved Hide resolved
node/db/src/rocks.rs Outdated Show resolved Hide resolved
node/db/tests/rocks_test.rs Outdated Show resolved Hide resolved
@austinabell austinabell mentioned this pull request Jan 3, 2020
Copy link
Contributor

@austinabell austinabell left a comment

Choose a reason for hiding this comment

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

Meant to approve before, but lgtm once #136 comes in to fix the CI

Edit: Review still valid

Copy link
Member

@ec2 ec2 left a comment

Choose a reason for hiding this comment

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

License. Otherwise lgtm

Copy link
Member

@ec2 ec2 left a comment

Choose a reason for hiding this comment

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

REMEMBER LICENSE

@dutterbutter
Copy link
Contributor

@GregTheGreek I think we should reference the db in the chain_store

@GregTheGreek
Copy link
Member Author

@GregTheGreek I think we should reference the db in the chain_store

What do you mean?

@dutterbutter
Copy link
Contributor

@GregTheGreek I think we should reference the db in the chain_store

What do you mean?

The RocksDb struct should be accessible in the chain store.

@GregTheGreek
Copy link
Member Author

seperate pr ?

@dutterbutter
Copy link
Contributor

separate pr ?

I will leave it up to you...it would be a small addition though.

@austinabell
Copy link
Contributor

austinabell commented Jan 6, 2020

separate pr ?

I will leave it up to you...it would be a small addition though.

Usage of it in this, especially since I am making changes on this in another PR, probably best to leave as is. I can add it in the one where I make changes to this (#132)

@GregTheGreek GregTheGreek merged commit ede60e7 into master Jan 6, 2020
@austinabell austinabell deleted the greg/db/setup branch January 10, 2020 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Generic Datastore
5 participants