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

kvdb-rocksdb: set format_version to 5 #395

Merged
merged 2 commits into from
Jul 27, 2020
Merged

kvdb-rocksdb: set format_version to 5 #395

merged 2 commits into from
Jul 27, 2020

Conversation

ordian
Copy link
Member

@ordian ordian commented May 20, 2020

The block_restart_interval is set in accordance with https://rocksdb.org/blog/2019/03/08/format-version-4.html.

Context: https://github.com/facebook/rocksdb/blob/a1523efcdf2f0e8133b9a9f6e170a0dad49f928f/include/rocksdb/table.h#L246-L271

This change is backwards compatible with rocksdb versions >= 6.6.

// This option only affects newly written tables. When reading existing
// tables, the information about version is read from the footer.

@dvdplm
Copy link
Contributor

dvdplm commented May 20, 2020

Context: https://github.com/facebook/rocksdb/blob/a1523efcdf2f0e8133b9a9f6e170a0dad49f928f/include/rocksdb/table.h#L246-L271

Given the outcome of our experiments with version 5 it seems likely that this also is effective only for new tables, do you agree?

@ordian
Copy link
Member Author

ordian commented May 20, 2020

Context: https://github.com/facebook/rocksdb/blob/a1523efcdf2f0e8133b9a9f6e170a0dad49f928f/include/rocksdb/table.h#L246-L271

Given the outcome of our experiments with version 5 it seems likely that this also is effective only for new tables, do you agree?

yeah, it's explicitly mentioned in version 3 and 4

// This option only affects newly written tables. When reading existing
// tables, the information about version is read from the footer.

Co-authored-by: David <dvdplm@gmail.com>
@dvdplm
Copy link
Contributor

dvdplm commented Jun 23, 2020

@NikVolf can you take a look at this please? Would be good to include this before we merge #402

@NikVolf
Copy link
Contributor

NikVolf commented Jul 27, 2020

Why is it needed?

Sorry, missed this thing for a while 😔

@ordian
Copy link
Member Author

ordian commented Jul 27, 2020

version 5 introduces improvements to bloom filter https://github.com/facebook/rocksdb/wiki/RocksDB-Bloom-Filter#full-filters-new-format

@ordian
Copy link
Member Author

ordian commented Jul 27, 2020

the default version in librocksdb-sys 6.7.4 (the latest published one) is 2 https://github.com/facebook/rocksdb/blob/27e593fbe10efa5a562ca79b1939ffb69d25163f/include/rocksdb/table.h#L275
in librocksdb-sys 6.10.1, which is merged in master, but not published, it's 4: https://github.com/facebook/rocksdb/blob/1482c8697c260cc12f7fce9f15401a85330d2ff1/include/rocksdb/table.h#L272

@NikVolf
Copy link
Contributor

NikVolf commented Jul 27, 2020

I hope this get tested on the database created without this update

@ordian
Copy link
Member Author

ordian commented Jul 27, 2020

I just tested this with openethereum, it worked.

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.

None yet

3 participants