Move RocksDB to git submodule

[vweevers]

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.

[lu4]

Yes, I've been just doing manual comparison of projects and it turned out that snappy libs are identical, so I thought that actual rebase could have helped

[lu4]

Lemme see the difference between this PR and thing I have on my local machine just in case check if nothing was missed...

[lu4]

Ok, here are the changes we've discussed in #140 thread. The pull request is targeted into your current PR branch https://github.com/Level/rocksdb/pull/142

[vweevers]

Remaining tasks:

• [x] Review
  • [x] Can probably remove 'rocksdb/util/' from include_dirs now
• [x] Merge https://github.com/Level/rocksdb/pull/142
• [x] Check .gitignore and .npmignore (we made some changes there, make sure paths still match)
• [x] Upgrade to at least https://github.com/facebook/rocksdb/commit/3b4d1b7a446dcf7cc6937db4f61734f7b0093a09 to avoid a regression of https://github.com/Level/rocksdb/pull/86
• [ ] Test compiling inside npm package (without git metadata being available)

[lu4]

@vweevers I've also noticed there's a high severity vulnerability found in dependency-check, it'll be good to probably upgrade dependencies?

Checking /Users/Lu4/Projects/rocksdb-ts/package.json
[====================] 25/25 100%

 node-gyp-build          ~4.1.0  →   ~4.1.1 
 coveralls               ^3.0.2  →   ^3.0.6 
 dependency-check        ^3.3.0  →   ^4.1.0 
 electron                ^6.0.0  →  ^6.0.10 
 level-concat-iterator   ^2.0.0  →   ^2.0.1 
 node-gyp                ^5.0.0  →   ^5.0.3 
 nyc                    ^14.0.0  →  ^14.1.1 
 prebuildify             ^3.0.0  →   ^3.0.4 
 standard               ^14.0.0  →  ^14.3.1 
 tape                   ^4.10.0  →  ^4.11.0 

[vweevers]

That's only a devDependency so hasn't got me worried. That said, upgrading should be easy (#128)

[vweevers]

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

[lu4]

@vweevers I'm doing final check, it takes 10 minutes to rebuild then 10 minutes to install, and then couple of minutes to check, I'm currently half way to install this beast

[vweevers]

10 minutes to rebuild then 10 minutes to install

Are you using npm run rebuild to rebuild? That should be the only step needed.

[lu4]

@vweevers Yes, I first do npm run rebuild then I go to my test project, and do npm uninstall rocksdb-ts && npm i ../rocksdb-ts, then I try to run it. The thing is that the issue I'm seeing is runtime you're unable to see it during rebuild.

[lu4]

ok, lemme push that into new branch, it takes too long...

[vweevers]

I see, you're testing it in another project? I recommend just doing npm run rebuild && npm t in this rocksdb repo; test it in isolation.

[lu4]

Here is the MR: https://github.com/Level/rocksdb/pull/143

[lu4]

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]

@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]

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]

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: https://github.com/Level/rocksdb/pull/141#issuecomment-533885344

[vweevers]

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

[MeirionHughes]

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]

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

[MeirionHughes]

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]

Yea, I picked this PR up because I was planning to release a major anyway for https://github.com/Level/community/issues/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]

5.0.0