[Question] Any chance bringing the forks together? about rust-rocksdb HOT 26 CLOSED

Yeah! ngaut's (being used for the tikv project) is the main one I would
like to pull in! Right now I'm backpacking in the Alps though and will be
away from a computer until the middle of October, but I can start to work
on this more when I get back! If you would like to work on this at all
before then just say so and I'll be able to review/merge from my phone at
some point every other day or so!

On Sep 29, 2016 07:49, "Mario Mueller" [email protected] wrote:

Hey there!

First, thanks for making this all happen, it helps me a lot in my daily
work!

Secondly: Is there a chance, that you/we can get all forks on one table
and see how this can be beneficial for the project? For example: You and
also https://github.com/ngaut/rust-rocksdb seem to have made pretty good
progress, but the branches are too diverged and I lack of knowledge to
merge them my self.

So I try and shoutout to @ngaut https://github.com/ngaut, @BusyJay
https://github.com/BusyJay and @zhangjinpeng1987
https://github.com/zhangjinpeng1987 to join the discussion if those two
forks should be one again?

Hope that helps somehow ...

Cheers,
Mario

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#69, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABb40J2MVp5hH2cPrBXiydB6lVEQw3wSks5qu1FRgaJpZM4KJmnu
.

from rust-rocksdb.

Hi, @xenji We would like to merge all of the changes to the repo. It's just there is a problem:
The new interfaces are not compatible with the previous version of rust-rocksdb.
And that could bother users who are already used rust-rocksdb in their project.

from rust-rocksdb.

I'm happy cutting a new major number for an api breaking new version

On Sep 30, 2016 07:57, "goroutine" [email protected] wrote:

Hi, @xenji https://github.com/xenji We would like to merge all of the
changes to the repo. It's just there is a problem:
The new interfaces are not compatible with the previous version of
rust-rocksdb.
And that could bother users who are already used rust-rocksdb in their
project.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#69 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABb40A2S_mwPEWeg-xb5CYgestkcgoI_ks5qvKS5gaJpZM4KJmnu
.

from rust-rocksdb.

Seems like a plan. Good to have you all in the conversation!

Should we revive this discussion and dive deeper when @spacejam is back from the Alps? I think this needs more than just a bit of talking, doesn't it?

from rust-rocksdb.

@spacejam If you stick to Semantic Versioning you don't even need to cut a new major version as long as you don't have a 1.0.0 release yet.

from rust-rocksdb.

Any updates on all this?

from rust-rocksdb.

Hey Everyone,

I'd just like to say, I'm keen to help out with improving the general quality of this wrapper (such as writing API docs and improving the tests). I also need some more features of RocksDB for my project, which I'm happy to put some of my own time in to implementing.

It looks like some of this work may have already been done in the other forks though. I'm not sure what I should do as my availability to help out will deminish after this week but any work I do now will probably conflict or duplicate with what has been done in the other forks.

I'm happy to help out in any way I can to get the changes in the forks pushed upstream so let me know if you need an extra pair of hands.

from rust-rocksdb.

Good job. @kaedroho
I think you may port all of the changes from my Repo which is currently used by TiKV project. We are still adding more and more features to that Repo. And i guess your porting process will break the current API.
Any thoughts? @spacejam

from rust-rocksdb.

Thanks @ngaut

Just been having a look. It appears the fork was made a while ago and both itself and upstream have had significant changes since then. This complicates things a little bit.

I reckon our best course of action is:

• Review changelog of ngaut's fork, create a list of pieces of work that have been done on that fork (me)
• Agree which items on that list should be ported and priority (everyone)
• For each feature, port it over to the upstream fork, add any docs/tests that may be missing, create a PR (me and anyone else who wants to help out)
• Review the PR and merge (@spacejam, @ngaut)

Does this sound OK?

from rust-rocksdb.

SGTM. @kaedroho

from rust-rocksdb.

Here's the list of the pull requests that have been submitted against ngaut's fork:

Code cleanup

• Cleanup https://github.com/ngaut/rust-rocksdb/pull/2
• Reorganise code https://github.com/ngaut/rust-rocksdb/pull/6
• Tiny cleanup https://github.com/ngaut/rust-rocksdb/pull/20
• Refactor options https://github.com/ngaut/rust-rocksdb/pull/38
• Cleanup test directory https://github.com/ngaut/rust-rocksdb/pull/40
• Make code more rusty https://github.com/ngaut/rust-rocksdb/pull/41
• Change line width to 100 https://github.com/ngaut/rust-rocksdb/pull/47

Static linking

• Support static linking https://github.com/ngaut/rust-rocksdb/pull/26
• Detect stdc++ automatically https://github.com/ngaut/rust-rocksdb/pull/27
• fix header not found https://github.com/ngaut/rust-rocksdb/pull/28
• Add ffi_try! to simplify error handling https://github.com/ngaut/rust-rocksdb/pull/46
• Use default c++ if specified https://github.com/ngaut/rust-rocksdb/pull/48

Iterators

• Export Iterator.valid() https://github.com/ngaut/rust-rocksdb/pull/7 (already merged)
• Make iterator not copy https://github.com/ngaut/rust-rocksdb/pull/9
• Refactor iterator https://github.com/ngaut/rust-rocksdb/pull/12
• Remove start key when create iterator https://github.com/ngaut/rust-rocksdb/pull/13 and https://github.com/ngaut/rust-rocksdb/pull/23
• Allow setting iterate option https://github.com/ngaut/rust-rocksdb/pull/16

Options

• Add compression option https://github.com/ngaut/rust-rocksdb/pull/8
• Add max_bytes_for_level option https://github.com/ngaut/rust-rocksdb/pull/10
• Add disable_wal option https://github.com/ngaut/rust-rocksdb/pull/11
• Add report_bg_io and compression_per_level options https://github.com/ngaut/rust-rocksdb/pull/19
• Add set_bloom_filter and set_cache_index_and_filter_blocks to BlockBasedOptions https://github.com/ngaut/rust-rocksdb/pull/25
• Add set_level_zero_file_num_compaction_trigger option https://github.com/ngaut/rust-rocksdb/pull/30
• Add set_iterator_upper_bound to ReadOptions https://github.com/ngaut/rust-rocksdb/pull/31
  • Not actually a RocksDB option
  • Methods now require ownership of ReadOptions (which prevents reuse)
• Add set_max_manifest_file_size option https://github.com/ngaut/rust-rocksdb/pull/33
• Add set_wal_recovery_mode option https://github.com/ngaut/rust-rocksdb/pull/34
  • follow up fix: https://github.com/ngaut/rust-rocksdb/pull/36
• Add enable_statistics and set_stats_dump_period_sec options https://github.com/ngaut/rust-rocksdb/pull/42
• Add set_compaction_filter option https://github.com/ngaut/rust-rocksdb/pull/43
• Add set_num_levels option https://github.com/ngaut/rust-rocksdb/pull/45
• Add get_statistics option https://github.com/ngaut/rust-rocksdb/pull/49

Miscellaneous

• Add flush and approximate size support https://github.com/ngaut/rust-rocksdb/pull/3
• Add get and get_cf methods to Snapshot https://github.com/ngaut/rust-rocksdb/pull/4
• Add delete files in range https://github.com/ngaut/rust-rocksdb/pull/14
  • Note: “files” should be pluralised. The RocksDB CAPI is wrong
• Add path method to DB https://github.com/ngaut/rust-rocksdb/pull/15
  • Q: Do DBs always have a path?
• Support get_property
  • https://github.com/ngaut/rust-rocksdb/pull/17
  • https://github.com/ngaut/rust-rocksdb/pull/18
• Introduce UnsafeSnap https://github.com/ngaut/rust-rocksdb/pull/21
• Add count and is_empty methods to WriteBatch https://github.com/ngaut/rust-rocksdb/pull/22
  • Q: Should count be len?
• update_cf https://github.com/ngaut/rust-rocksdb/pull/24
• Add list_column_families API https://github.com/ngaut/rust-rocksdb/pull/37
• Make compact_range arguments optional https://github.com/ngaut/rust-rocksdb/pull/44

from rust-rocksdb.

Here's my opinions.

In general, I didn't notice a single change that had documentation. Some parts do not have tests and the tests that are there could be improved.

Code cleanup

There has been some major refactoring and cleanups in the fork, but upstream has been heavily changed too. I don't think it's worth, and may not even be feasible to port these.

If we must reproduce these cleanups, we should probably get them out of the way first so avoid merge conflicts in new changes.

Static linking

I think it would be great to get this upstream. I had problems with jemalloc segfaulting under a dynamically linked RocksDB.

The ethcore fork have implemented this as well. They appear to have done it differently and also have Windows support so I think it's worth looking into both implementations.

Iterators

There's been a lot of work around refactoring the iterator API and I disagree with some of the design decisions. I think we should discuss this part of the API before making any changes.

Options

Most of these changes follow a similar pattern by just exposing new functions in the Options struct. We could port all of these in one go. I had to do some digging to find out what some of them do, so I think it would be a good idea to also add docstrings for all of them as we do the porting.

There's one change that I disagree with though, https://github.com/ngaut/rust-rocksdb/pull/31. This adds an upper-bound to range iterators but it's not part of the standard RocksDB API. The PR also changes the read methods to consume the ReadOptions object which prevents reuse.

from rust-rocksdb.

Thanks for your excellent work. @kaedroho

About Options:
Upper-bound iterator is actually one of the standard RocksDB API. See https://github.com/facebook/rocksdb/blob/master/include/rocksdb/options.h#L1447
And it's really useful in some scenarios. I insist that we should keep it.

Others sounds great to me.

from rust-rocksdb.

Thank you very much for stepping forward to help with this, @kaedroho!

I can imagine the upper bound being a nice way to help with things like multiplexing higher level lexicographic ranges of sharded databases, and getting higher performance than filtering results after the fact. I'm in favor of keeping it.

In general I'm in favor of making the merge path for @ngaut's changes as seamless as possible, as he is doing a ton of great work that I'd love to merge in over time. To that end, I'm generally in favor of keeping whatever API semantics his version has (maybe rethinking ownership of ReadOption though), breaking the ones here where necessary, adding proper documentation (I can help with this over time).

I can commit a couple hours per week to this to help this effort, but it may look more like a whole day per month or something like that due to random traveling. Since you want to cram work into this week, I'll try to pay extra close attention to this to clear blockers on my end to your progress.

from rust-rocksdb.

Upper-bound iterator is actually one of the standard RocksDB API.

Ahh, my mistake! It looks like a bit of dead code got through on that pull request (which led me to think this was custom). See: https://github.com/ngaut/rust-rocksdb/pull/31/files#diff-15fa9749208206c5a07bbe0cc9aa11abR45

This new attribute is the reason why the ownership rules had to change, but it doesn't look like it was ever used anywhere, so I think it can just be removed. I'll just port the set_iterate_upper_bound method across.

from rust-rocksdb.

Thanks for your excellent work. @kaedroho
Thank you very much for stepping forward to help with this, @kaedroho!

No problem! This wrapper has been really useful to me so far, glad I can help make it better!

In general I'm in favor of making the merge path for @ngaut's changes as seamless as possible, as he is doing a ton of great work that I'd love to merge in over time.

I agree, the TiKV team have been making some really useful changes. But I think it's a shame that both TiKV and Ethcore have decided that they must maintain their own forks in isolation of each other. Both projects have implemented static linking (a huge bit of work) when only one of them really needed to and new projects in the future will not benefit from either of their efforts!

I hope this effort would be a step towards bringing all the teams together, so we can all work on the same codebase and benefit from each other's contributions.

I've just submitted the first batch of work which pulls across most of the changes to the ReadOptions struct. See #72

Next, I'm going to have a look at the static linking implementations. I'll probably write up an issue describing what I've learned and propose an implementation before starting work.

from rust-rocksdb.

Sorry for the delay (had to leave my desk for a while). Here's my quick analysis of how the two forks have implemented static linking.

tl;dr The only notable difference between the two is how they both build RocksDB. I prefer the approach taken in ethcore because of it's cross-plafrom support and it doesn't do any downloads during compilation.

Please let me know if you disagree with my observations/opinions. The approach I'd like to take is to copy the implementation from the ethcore fork, tweak it to fix any compatibility issues and make it use git-submodules instead of bundling RocksDB/snappy. Please let me know if that sounds OK to you.

What they both do

• They both have a sub-crate suffixed with -sys for FFI bindings
• They both use a build.rs file for compiling RocksDB
• They both link against a specific version of RocksDB

What they do differently

• ngaut downloads and compiles bz2, libz, lz4, snappy and RocksDB
• ethcore bundles the RocksDB and snappy code in the crate and builds locally

My opinions:

I prefer the way the ethcore fork works here. Performing extra downloads during a build feels very ugly to me, download links may break in the future and we can’t be sure the build server has an internet connection.

I don’t like having full copies of dependencies bundled in the repo, I think git-submodules would be a good fit here instead.

The ngaut fork builds with bz2, libz, lz4 and snappy whereas the ethcore fork only builds with snappy. This would obviously, improve compile time considerably but I wonder is snappy is sufficient for all use cases?

• ngaut’s build.rs file calls a shell script which downloads each project, then builds them using their Makefile
• ethcore’s build.rs uses the gcc-rs crate to perform the build (not using the project’s makefiles, or any external tools)

My opinions:

Again, I prefer the approach the ethcore fork has taken here. Using a shell script would make cross-platform support very difficult. Different distributions of Linux may not have all the required tools installed to run it and there are many different shells to take into account. It will need to be completely reimplemented for Windows support.

Ethcore keeps all of it’s build logic inside the build.rs file. This file can be built and run on any platform that Rust supports. Despite the name, the gcc-rs crate works well with Visual C++ on Windows (the ethcore fork has Windows support).

The only downside I can think of with the ethcore approach is that every .cc file needs to be listed in build.rs. This list may need to be updated when RocksDB is updated. The linker should give us an error message when this list is incorrect so there’s little chance of mistakes.

from rust-rocksdb.

Actually we thought about using git submodule and gcc-rs at first. But it seems ugly to me to track all the .cc files again which was done in the Makefile already. And git-submodule is too slow for me to clone a new repository from the beginning.

I think almost all the linux distribution and OS X have bash support. Windows can use PowerShell get the job done, though It's not implemented yet in the ngaut branch.

If build server doesn't have a network connection, then it will be hard for it to build a rust project. Because the crates registry, crates source are needed to be updated (or initialized) via a connection to github.com and crates.io.

Please note that, gcc-rs just wraps a gcc process, so it depends on gcc compiler. Rocksdb can't be compiled without any external tools (yet).

from rust-rocksdb.

Thanks for chiming in @BusyJay!

And git-submodule is too slow for me to clone a new repository from the beginning.

Ahh, good point. Having to pull down a 6000-commit history when you only need the state at one time is a bit heavy. This won't be a problem when rust-rocksdb is installed from crates.io though as the checked out submodules will be bundled in the crate at the point of publishing it.

But it seems ugly to me to track all the .cc files again which was done in the Makefile already.

I agree, but it's the lesser of two evils in my opinion

If build server doesn't have a network connection, then it will be hard for it to build a rust project. Because the crates registry, crates source are needed to be updated (or initialized) via a connection to github.com and crates.io.

I'll elaborate on this a bit

1. It is possible that somebody would want to run a local registry (they may be going on an airplane, or work in a high-security environment). In this case, the user does not expect any network connections would be required to install the crate. See: http://doc.crates.io/source-replacement.html#local-registry-sources

2. When Cargo's --frozen flag is used, Cargo will refuse to run if it needs to make a network connection. So if Cargo did run, the user would expect that no network connections were made, but this isn't the case with the ngaut branch, as it stands, which will proceed to make four downloads at compile time. See: http://doc.crates.io/faq.html#how-can-cargo-work-offline

3. A lot of people cache the crate sources on their build server (including this project), but not the built versions. But every time they compile, they will still have to refetch these four sources as they are only considered by Cargo as a side-effect of the build script and not something worth caching. This, again, is not what the user would expect.

4. It adds four build-time dependencies that are outside of the cargo ecosystem. If any of those sites they are hosted on go down, nobody will be able to build.

from rust-rocksdb.

Would you consider a PR that changed the FFI module to something like https://github.com/alexreg/rocksdb-sys/blob/master/src/lib.rs? This is up-to-date with RocksDB (v4.11.2), and stays faithful to the C API – no renamings of types, and arguably more complete tests (see https://github.com/alexreg/rocksdb-sys/blob/master/tests/c_test.rs in the same repo, since they've been ported from the C tests). I'd also recommend forking the FFI stuff into a separate _sys crate, even if it lies within the same repo.

Note that type aliases could then be given where no interop is needed. And for the enums, a simple numerical mapping could be made.

from rust-rocksdb.

@alexreg I've forked the FFI module into a separate -sys crate in my "implement static linking" PR (#80) submitted earlier today. Hopefully this will be looked at soon!

Those FFI bindings are much more complete than what we currently have (implementing 274 library functions vs 116 in this repo), and the test suite is a massive bonus. I think pulling this in would be very helpful*.

But I'd still like the common, everyday API to feel "rusty", following Rust's naming conventions. The FFI module could become a behind the scenes thing which follows RocksDB's naming conventions (and is public for those who want more control). The enums currently defined in FFI will probably have to be redefined somewhere else.

* I'm not a maintainer though, @spacejam could give an official answer to your question

from rust-rocksdb.

@kaedroho Thanks, that's good to know. Maybe if your PR gets accepted soon, and @spacejam is happy with this proposed reworking of the FFI bindings, then I could have a go implementing it on top of the latest changes.

from rust-rocksdb.

@kaedroho How did you get librocksdb to link (statically) against libjemalloc by the way? Specifically, the version of libjemalloc.a that Rust uses.

Edit:
Got it. There was a conflict between the Rust version of jemalloc and the system jemalloc on my system. That wreaked havoc, so the solution was to link against the system malloc. Here's my build.rs that offers a simple (non-Windows) alternative. Feel free to ignore it, but it may be of interest to someone.

https://gist.github.com/1ae9b2100ccd75c3efbb6e4d0c3b6fbd

from rust-rocksdb.

Peeps, that is how open source collaboration should work! Thank you, you are awesome! :D

from rust-rocksdb.

I'd like to continue to pull in changes over time from the various forks, so this is by no means over, but for now we've wrapped up a nice big chunk.

Today 0.5.0 will be released, which includes the great work you've all done in this issue, and others!

I've been pleasantly surprised by the amount of contribution that this issue has stirred up, and blown away by the amount of effort put in by @kaedroho and @alexreg ! Everyone who contributed, this project is in a much nicer place now after your efforts :) Thank you!

from rust-rocksdb.

Glad to help. Thanks likewise for maintaining this project so diligently. :-)

On 20 Nov 2016, at 11:03, Tyler Neely [email protected] wrote:

Closed #69 #69.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #69 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAEF3Mtu1brGHHFw4NxgJS6YIRgS6koiks5rACkPgaJpZM4KJmnu.

from rust-rocksdb.