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

Move RocksDB to git submodule #141

Merged
merged 3 commits into from
Apr 10, 2021
Merged

Move RocksDB to git submodule #141

merged 3 commits into from
Apr 10, 2021

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Sep 22, 2019

Forked from #82. Closes #140.

I've yet to review it myself. I only rebased it, and removed the hardcoded sha1 for build_version.cc, which I know we will forget to update at some point - so I prefer not having it.

@vweevers vweevers added the semver-patch Bug fixes that are backward compatible label Sep 22, 2019
@vweevers vweevers mentioned this pull request Sep 22, 2019
@lu4

This comment has been minimized.

@lu4

This comment has been minimized.

@lu4

This comment has been minimized.

@vweevers
Copy link
Member Author

vweevers commented Sep 22, 2019

Remaining tasks:

@lu4

This comment has been minimized.

@vweevers

This comment has been minimized.

@vweevers vweevers mentioned this pull request Sep 22, 2019
2 tasks
@vweevers
Copy link
Member Author

@lu4 How's it going with the RocksDB upgrade? Do you wanna take it from here?

@lu4

This comment has been minimized.

@vweevers

This comment has been minimized.

@lu4

This comment has been minimized.

@lu4

This comment has been minimized.

@vweevers

This comment has been minimized.

@lu4
Copy link
Contributor

lu4 commented Sep 22, 2019

Here is the MR: #143

@lu4
Copy link
Contributor

lu4 commented Sep 22, 2019

For greater verbosity I've commented out the files excluded from build, plus I moved the port files and added build_version.cc in the end of rocksdb.gyp, also the tests currently failing, looking into what's wrong with them

@lu4
Copy link
Contributor

lu4 commented Sep 22, 2019

@vweevers ok, as per https://github.com/facebook/rocksdb/search?q=ods&unscoped_q=ods it turns out that I've included some strange ODS piece of code that has some relation to RocksDB stats. It probably shouldn't be included into production build. Investigating for potential reasons...

@MeirionHughes
Copy link
Member

Its a shame this stalled - I was just looking at rockdb and it turns out they enabled 1 primary write + secondary read process support, in a newer version.

Was there something drastic that killed this PR?

@vweevers
Copy link
Member Author

1 primary write + secondary read process support

Sweet!

Was there something drastic that killed this PR?

We just need someone to pick up the work. We can start by reviewing this PR, merging it, then continuing with the tasks listed here: #141 (comment)

@vweevers vweevers mentioned this pull request Apr 10, 2021
@vweevers vweevers added semver-major Changes that break backward compatibility and removed semver-patch Bug fixes that are backward compatible labels Apr 10, 2021
@vweevers
Copy link
Member Author

I updated this to RocksDB 6.17.3, fixed the few remaining issues, and squashed the commits. Thank you @filoozom @lu4!

@vweevers vweevers merged commit 060d182 into master Apr 10, 2021
@vweevers vweevers deleted the rocksdb-submodule2 branch April 10, 2021 20:12
@MeirionHughes
Copy link
Member

tested local clone with yarn link and yarn link rocksdb and it hasn't broken my work's app. my electron rebuild fails - but I suspect that is because I'm in a mono repo and it cannot find a relative path to node-gyp while using the link.

@vweevers
Copy link
Member Author

FWIW, rebuilding for Electron should not be needed because the N-API builds are runtime agnostic (unless you have an old electron version.

@MeirionHughes
Copy link
Member

MeirionHughes commented Apr 11, 2021

This will need a major version bump and a warning - its fine with an old database, but that auto-updated database won't be compatible with an older version of rocksdb. maybe due to https://rocksdb.org/blog/2019/03/08/format-version-4.html ?

Corruption: Unknown Footer version. Maybe this file was created with newer version of RocksDB?
Error [ReadError]: Corruption: Unknown Footer version. Maybe this file was created with newer version of RocksDB?

@vweevers
Copy link
Member Author

Yea, I picked this PR up because I was planning to release a major anyway for Level/community#98.

Will add a note to our UPGRADING.md with a link to their HISTORY.md. I think the breaking version is 6.9.0:

Updated default format_version in BlockBasedTableOptions from 2 to 4. SST files generated with the new default can be read by RocksDB versions 5.16 and newer, and use more efficient encoding of keys in index blocks.

@vweevers
Copy link
Member Author

5.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Changes that break backward compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RocksDB version upgrade
4 participants