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

Change iterators to return Result #648

merged 14 commits into from Aug 5, 2022


Copy link

@mina86 mina86 commented Jun 28, 2022

Currently, Iterator implementation for DBIteratorWithThreadMode
produce (key, value) pairs on iteration. When it stops (i.e. next
method returns None) the user is expected to call status method to see
if the iteration finished normally or due to an error. This is, dare
I say, not very idiomatic.

Consider for example std::io::BufRead::lines or std::fs::read_dir
methods. They return iterators whose Items are std::io::Result<_>
objects. On error the error is returned as an item from the iterator
rather than finishing the iterator and providing separate way for the
user to check whether there was an error.

Change Iterator implementations for DBIteratorWithThreadMode such that
it mimic this behaviour. Rather than returning a value and expecting
user to test status once iterator finishes, the iterators now return
Result<_, Error>. With that also remove valid and status methods
(since the information is already returned via iteration and it makes
the code slightly simpler).

Similarly, for the same reasons, change Item of DBWALIterator to be
Result<_, Error> as well.

This is obviously an API breaking change. Anyone using iterators will
have to change their code to handle the errors.

mina86 added 2 commits Jun 28, 2022
… pair

The method is morally equivalent to calling `key` and `value` methods
separately however it reduces number of times validity of the iterator
needs to be checked.  Since that check goes through ffi layer and
calls a virtual method it doesn’t optimise well.  Therefore, reducing
number of times the `valid` method is called optimises the code a wee
Copy link

stanislav-tkach commented Jun 28, 2022

I really like this idea.

Copy link
Contributor Author

mina86 commented Jun 28, 2022

SG. I’ll try to finish this up by the end of the week.

mina86 added 2 commits Jun 29, 2022
Firstly, introduce helper assertions which reduce code repetition.
They do it by encapsulating verifying output of the iterator into
a single location.  This will be useful in upcoming commit which
changes the Item type of the iterator.

Secondly, keep keys and values declared at the start of some of the
tests as slices without boxing them.  Doing the latter introduces
conveluted syntax such as `&*k1`; keeping them as slices simplifies
the syntax and also allows them to be converte into constsants.

Lastly, replace `cba` helper methods with `pair` which construct
a pair of boxed slices.  This removes some repetition since after
the previous point `cba` was only ever used in a pair.
@mina86 mina86 changed the title RFC: change iterators to return Result Change iterators to return Result Jun 29, 2022
Copy link
Contributor Author

mina86 commented Jun 29, 2022

I think this is done. The other two PRs I have pending (#647 and #652) should go first as they will make this one much smaller.

Copy link
Contributor Author

mina86 commented Jul 12, 2022

This is ready to land.

Copy link
Contributor Author

mina86 commented Aug 1, 2022

Friendly ping.

tests/ Outdated Show resolved Hide resolved
mina86 and others added 3 commits Aug 1, 2022
Copy link

aleksuss commented Aug 3, 2022

Friendly ping.

Looks good. But I will merge it after #565

@aleksuss aleksuss merged commit 8710105 into rust-rocksdb:master Aug 5, 2022
6 checks passed
@mina86 mina86 deleted the result branch Aug 5, 2022
vldm pushed a commit to velas/rust-rocksdb that referenced this pull request Sep 8, 2022
msmouse pushed a commit to aptos-labs/rust-rocksdb that referenced this pull request Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

3 participants