Streamline kvdb API and upgrade rocksdb to 0.19

[ordian]

The rocksdb upgrade seems non-breaking, but it uses tikv-jemalloc-sys transitively which links to the native library jemalloc.
It's about time we release a new version of each crate anyway.

Depends on: #662.

[niklasad1]

I'm aboard to update these deps but it looks like the API for rocksdb::DBIterator has changed to Result<Option<_>> which needs to fixed in order to compile

[ordian]

Yup, submitted a PR too quickly, but in the process of fixing this realized how to remove owning_ref and simplify the code 💡
After submitting the PR, will fix this one.

[ordian]

The compilation fix is in 2957b4e An alternative would be to change the kvdb to have Item = DBResult<KeyValuePair> in order to propagate the error to the caller.

I don't have a strong preference between these two.

[cheme]

Could also consider changing kvdb trait.
get return io_result, get_by_prefix does not, iter does not, maybe all should return result (even iterator step), I think it is the case on parity_db.
(could be part of #661)

[dvdplm]

Can you rebase now that #662 is merged for easier reviewing?

[ordian]

Could also consider changing kvdb trait. get return io_result, get_by_prefix does not, iter does not, maybe all should return result (even iterator step), I think it is the case on parity_db. (could be part of #661)

Thanks for this. I was not sure whether to make more breaking changes or keep as is: #661 (comment), but I think having an iterator API that swallows errors might be a bad design and it's better to let the use bail out ASAP if needed (I imagine that'd be the default). Will make those changes to kvdb to streamline the API as well.

[cheme]

Could also consider changing kvdb trait. get return io_result, get_by_prefix does not, iter does not, maybe all should return result (even iterator step), I think it is the case on parity_db. (could be part of #661)

Thanks for this. I was not sure whether to make more breaking changes or keep as is: #661 (comment), but I think having an iterator API that swallows errors might be a bad design and it's better to let the use bail out ASAP if needed (I imagine that'd be the default). Will make those changes to kvdb to streamline the API as well.

yes this https://github.com/paritytech/polkadot/blob/0879bdbcd93ae96602d42e1c2f9fd54c752eace5/node/subsystem-util/src/database.rs#L147 was me being lazy to change the trait, seeing that rocksdb deps also return result, changing trait sounds right to me.

[ordian]

Could also consider changing kvdb trait. get return io_result, get_by_prefix does not, iter does not, maybe all should return result (even iterator step), I think it is the case on parity_db. (could be part of #661)

Thanks for this. I was not sure whether to make more breaking changes or keep as is: #661 (comment), but I think having an iterator API that swallows errors might be a bad design and it's better to let the use bail out ASAP if needed (I imagine that'd be the default). Will make those changes to kvdb to streamline the API as well.

yes this https://github.com/paritytech/polkadot/blob/0879bdbcd93ae96602d42e1c2f9fd54c752eace5/node/subsystem-util/src/database.rs#L147 was me being lazy to change the trait, seeing that rocksdb deps also return result, changing trait sounds right to me.

I don't blame you here. As you can see I was hesitant to make these breaking changes here myself. We need to improve our release and upgrade process so people are less afraid of making breaking changes.

Updated the PR, please take a look.

[ordian]

Can you rebase now that #662 is merged for easier reviewing?

@dvdplm Could you take a look please?

[niklasad1]

Not sure whether folks care about whether the key was not found or an error occurred.... i.e, you could convert both Option::None and Result:Error to false

Further it also possible use get_by_prefix if one wants to know the cause..

[ordian]

True, but it was also inconsistent with has_key method. I believe we should try to report the IO error to the user as soon as it happens and not swallow it in the lib. It might make it less ergonomic, but with ? shouldn't be much of a problem.

[niklasad1]

nicely done, looks good to me I had a couple optional minor comments...