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

Add rocksdb #12

Merged
merged 5 commits into from
Sep 18, 2019
Merged

Add rocksdb #12

merged 5 commits into from
Sep 18, 2019

Conversation

Stumble
Copy link
Contributor

@Stumble Stumble commented Jul 30, 2019

tendermint/tendermint#3831

Using my fork of gorocksdb from https://github.com/tecbot/gorocksdb for two reasons: 1. The upstream repo encourages vendoring for API stability. 2. The default LDFLAGS setting in upstream repo enforces linking unnecessary compression libraries(https://github.com/tecbot/gorocksdb/blob/162552197222a834c6857385ae890e4decdd5049/dynflag.go#L5), so that if the library is not installed, link will fail. IMHO, tendermint should fork this wrapper and use it here.

To run tests:
First, install rocksdb,

homedir="$PWD"
sudo apt-get update
sudo apt-get install -y build-essential gcc g++ make libgflags-dev libsnappy-dev
wget -O rocks.tar.gz https://github.com/facebook/rocksdb/archive/v6.1.2.tar.gz && \
  tar -zxvf rocks.tar.gz && \
  cd rocksdb-6.1.2/ && \
  make static_lib -j4 && \
  sudo cp librocksdb.a /usr/local/lib/ && \
  cd include/ && \
  sudo rm -rf /usr/local/include/rocksdb && \
  sudo cp -r rocksdb /usr/local/include/ && \
  sudo ldconfig
cd "$homedir"
rm rocks.tar.gz
rm -rf rocksdb-6.1.2

Run tests:

GO_LDFLAGS="-lsnappy" go test -v -tags "rocksdb"

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍 looking good. left a few comments. would be great to have more tests!

db/db.go Outdated Show resolved Hide resolved
db/c_level_db_test.go Outdated Show resolved Hide resolved
db/rocks_db.go Outdated
ro := gorocksdb.NewDefaultReadOptions()
wo := gorocksdb.NewDefaultWriteOptions()
woSync := gorocksdb.NewDefaultWriteOptions()
woSync.SetSync(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

what this line does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean these variables? read/write options?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean woSync.SetSync(true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

woSync.SetSync(true) prepares a sync write option. https://godoc.org/github.com/tecbot/gorocksdb#WriteOptions.SetSync

db/rocks_db.go Outdated
key = nonNilBytes(key)
res, err := db.db.Get(db.ro, key)
if err != nil {
res.Free()
Copy link
Contributor

Choose a reason for hiding this comment

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

why not put this above with defer? that way we'll avoid duplication s.Free() in moveSliceToBytes and here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

turns out res.Free() when err != nil is not necessary, removed. I would prefer to have a function that moves c bytes to go slice.

@melekes
Copy link
Contributor

melekes commented Aug 2, 2019

Using my fork of gorocksdb from https://github.com/tecbot/gorocksdb for two reasons:

Have you tried to contribute back to original lib?

@Stumble
Copy link
Contributor Author

Stumble commented Aug 5, 2019

Using my fork of gorocksdb from https://github.com/tecbot/gorocksdb for two reasons:

Have you tried to contribute back to original lib?

Nope, as vendoring was suggested for gorocksdb anyway. I just sent an issue in the upstream, let's see how it goes.

@Stumble
Copy link
Contributor Author

Stumble commented Aug 9, 2019

@melekes , the upstream has not respond to the issue yet. tecbot/gorocksdb#167

@Stumble
Copy link
Contributor Author

Stumble commented Aug 10, 2019

db.go Outdated Show resolved Hide resolved
Co-Authored-By: Anton Kaliaev <anton.kalyaev@gmail.com>
@melekes melekes merged commit 37a6176 into tendermint:master Sep 18, 2019
@tac0turtle
Copy link
Contributor

@Stumble it seems gorocksdb has picked up steam could you rebase your fork until your PR get merged?

@Stumble
Copy link
Contributor Author

Stumble commented Oct 17, 2019

@marbar3778 The PR is merged, I suggest we switch to the original branch.

melekes added a commit to tendermint/tendermint that referenced this pull request Dec 11, 2019
Closes #3831

The support for rocksdb was added a while back in
tendermint/tm-db#12. This commit merely updates
the documentation.
melekes added a commit to tendermint/tendermint that referenced this pull request Dec 11, 2019
Closes #3831

The support for rocksdb was added a while back in
tendermint/tm-db#12. This commit merely updates
the documentation.
faddat pushed a commit to notional-labs/tm-db that referenced this pull request May 8, 2022
tac0turtle added a commit that referenced this pull request Aug 25, 2022
* Create codeql-analysis.yml

* Update codeql-analysis.yml

Co-authored-by: Marko <marbar3778@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants