Updated API for compatibility with latest rocksdb

[alexreg]

Made API compatible with rocksdb v6.29.3.

Removed following properties of ColumnFamilyOptions, which have been deprecated for some time. (They cause issues with Cython right now, and will be obsolete in v7 anyway.)

• soft_rate_limit
• hard_rate_limit
• rate_limit_delay_max_milliseconds
• purge_redundant_kvs_while_flush
• skip_log_error_on_recovery

Resolves #11.

[NightTsarina]

Hi @alexreg, thanks for the PR!

I have created a branch and PR to make the github CI test buiilding the library against the newer versions of librocksdb. Could you rebase this branch over it so we can verify all works?

Independently of that, I see that there is a breaking change that is already making the build fail against the older versions of librocksdb, which we might need to support for a while. Do you think it would be possible to make this change backwards compatible?

[NightTsarina]

One further comment: after making the changes for the CI, I see that the builds are succeeding with v6.29.3, without applying your change.. Is the CI doing something wrong, or we don't actually need this change?

[alexreg]

@NightTsarina The CI must be doing something wrong, because the API for 6.29.3 removed a few bits of the API that Python was trying to bind against. Let me check...

[alexreg]

One of the notable differences is that rocksdb/utilities/backupable_db.h got renamed to rocksdb/utilities/backup_engine.h in a recent version of rocksdb (not sure which).

An interesting approach (given you don't want to compile a fixed version of rocksdb along with python-rocksdb) would be to actually query which version of rocksdb is available on the system, and adjust the Python bindings accordingly. I believe Cython can do that!

P.S. Did you already remove the branch you created for testing/CI? The link is stale now.

[alexreg]

So, I've been doing some investigating, and come to a few realisations:

• The deprecated rocksdb/utilities/backupable_db.h still exists as a proxy for rocksdb/utilities/backup_engine.h, so we can still use that for now, though going forwards you'll want to change to backup_engine.h. (The old header file is removed in rocksdb 7.0).
• Various members of ColumnFamilyOptions have been deprecated for some time, and those are the ones I've removed. They're removed completely from the underlying rocksdb in 7.0.
• We can use pkgconfig (specifically the Python package for it) to try to auto-configure compiling & linking against rocksdb when possible. If not, we fall back and use the previous manual configuration, though I added a bit of functionality so you can specify the include and lib paths to search. This is necessary in particular on macOS with Homebrew, where rocksdb is installed in /opt/homebrew and without a pkg-config file.

I also put in a few comments in the backup.pxd file about how to update it in the near future.

Hopefully all tests should pass now.

[alexreg]

Seems like just some tests are failing now, which use the old ColumnFamilyOptions members.

[NightTsarina]

Great work, thanks!

There are a couple of pending issues, but I think we can merge this soon.

CC: @dato and @FdelMazo to see what they think.

[alexreg]

Thanks for the review. I just fixed the issues with the tests and CI, so hopefully everything will pass now!

[alexreg]

Note: Cython seems to insist on using the first signature for BackupEngine::Open, so we do in fact need to flip the order of the first two parameters to the current one.

[alexreg]

Almost all passing now... It seems the Ubuntu package for python-pkgconfig is old, and missing a bit of the API that setup.py uses. Can we install using pip instead, maybe?

[dato]

LGTM.

It would be great if the commit,
or the merged PR description, would enumerate the properties that where deleted.

[alexreg]

@dato Sure, I'll do that now.

[rollue]

Looking forward to this PR merge so much

[NightTsarina]

@alexreg In case you missed it, last week I've sent you a PR (alexreg#1) to fix the last remaining issues.

[alexreg]

@NightTsarina Thank you. I didn't get notified about it for some strange reason. I added your commit now!

[alexreg]

It's up to you of course, but maybe we could also add a brief note in the README about manually setting INCLUDE_PATH and LIBRARY_PATH? That is, it is needed if a) rocksdb lib & headers are not in the default C compiler path, and b) either pkgconfig is not installed or the rocksdb pkgconfig definition is not present. This is notably the case with macOS and Homebrew, and possibly other situations too.

[alexreg]

Ugh, the situation is complicated now by the latest rocksdb release being 7.0.3, which Homebrew uses. This has dropped the rocksdb/utilities/backupable_db.h file altogether (no longer just deprecated).

[NightTsarina]

It's up to you of course, but maybe we could also add a brief note in the README about manually setting INCLUDE_PATH and LIBRARY_PATH? That is, it is needed if a) rocksdb lib & headers are not in the default C compiler path, and b) either pkgconfig is not installed or the rocksdb pkgconfig definition is not present. This is notably the case with macOS and Homebrew, and possibly other situations too.

Oh yes, definitely, I missed that!

The tests are finally passing (yayy!!!), so let's merge this now. Could you send a follow-up PR with the documentation patch?

Ugh, the situation is complicated now by the latest rocksdb release being 7.0.3, which Homebrew uses. This has dropped the rocksdb/utilities/backupable_db.h file altogether (no longer just deprecated).

That is a different major version, I don't think it would be a good idea to switch right away, considering we still have quite some unfinished work in 6.x. Does homebrew not allow you to stay with an older version?

I think it might make sense to just compile it from source, like we do here for actions, and it is quite straightforward.. We could add it to the README too, with detailed instructions for macOS users.

[alexreg]

Ugh, the situation is complicated now by the latest rocksdb release being 7.0.3, which Homebrew uses. This has dropped the rocksdb/utilities/backupable_db.h file altogether (no longer just deprecated).

@rollue FYI This is probably the source of your issue.

[alexreg]

@NightTsarina Great, thanks for merging.

I agree... I'll just compile from source for now. (Homebrew only has the latest version for most software.)

I actually started porting to v7, but didn't get too far.