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

badarg in eleveldb:iterator_move(Ref, prev) after 1.2.2p5 #52

Closed
uwiger opened this issue Feb 5, 2013 · 12 comments
Closed

badarg in eleveldb:iterator_move(Ref, prev) after 1.2.2p5 #52

uwiger opened this issue Feb 5, 2013 · 12 comments

Comments

@uwiger
Copy link

uwiger commented Feb 5, 2013

I have yet to run this to ground, but using versions after 1.2.2p5, our test suites fail with a badarg when calling eleveldb:iterator_move(Ref, prev). Reverting back to 1.2.2p5 fixes the problem.

@uwiger
Copy link
Author

uwiger commented Feb 5, 2013

The following test works with 1.2.2p5, but not with HEAD:

-module(ltest).

-export([t/0]).

t() ->
    os:cmd("rm -rf ltest"),  % NOTE
    {ok, Ref} = eleveldb:open("ltest", [{create_if_missing, true}]),
    try
    eleveldb:put(Ref, <<"a">>, <<"x">>, []),
    eleveldb:put(Ref, <<"b">>, <<"y">>, []),
    {ok, I} = eleveldb:iterator(Ref, []),
    {ok, <<"a">>, <<"x">>} = eleveldb:iterator_move(I, <<>>),
    {ok, <<"b">>, <<"y">>} = eleveldb:iterator_move(I, next),
    {ok, <<"a">>, <<"x">>} = eleveldb:iterator_move(I, prev)
    after
    eleveldb:close(Ref)
    end.

With current HEAD:

Eshell V5.9.2  (abort with ^G)
1> c(ltest).
{ok,ltest}
2> ltest:t().
** exception error: bad argument
     in function  eleveldb:async_iterator_move/3
        called as eleveldb:async_iterator_move(undefined,<<>>,prev)
     in call from eleveldb:iterator_move/2 (src/eleveldb.erl, line 188)
     in call from ltest:t/0 (ltest.erl, line 14)

Using bit bisect, I found that commit a465386 seemed to break iterators, but that commit gives another error:

Eshell V5.9.2  (abort with ^G)
1> ltest:t().
** exception error: no match of right hand side value {error,invalid_iterator}
     in function  ltest:t/0 (ltest.erl, line 14)

As reference, this is how it looks with 1.2.2p5:

Eshell V5.9.2  (abort with ^G)
1> c(ltest).
{ok,ltest}
2> ltest:t1().
{ok,<<"t:a">>,<<"x">>}
3> ltest:t2().
{ok,<<"t:a">>,<<"x">>}
4> ltest:t3().
{ok,<<"a">>,<<"x">>}

I'm running on a Mac, OSX 10.7.5, with OTP R15B02.

@matthewvon
Copy link
Contributor

Prev() was intentionally left broken. The theory was that no one was using it and it would be one less thing to test. Do you actually use Prev(), or just have a test for it?

@uwiger
Copy link
Author

uwiger commented Feb 5, 2013

Prev is crucial for us, so we must stay with 1.2.2p5 until it's fixed.

@matthewvon
Copy link
Contributor

Ok. Once you have a position via Seek or SeekLast, do you typically call Prev() once or many times? Your answer will influence the nature of the Prev support.

@uwiger
Copy link
Author

uwiger commented Feb 5, 2013

Calling it once is the more common scenario, but there are use cases where prev is called repeatedly.

@uwiger
Copy link
Author

uwiger commented May 10, 2013

What is the status of this ticket? I just tried a recent version of eleveldb, and the badarg remains.

The use case I'm looking at currently iterates a user-specified number of times using prev.

@matthewvon
Copy link
Contributor

Our unannounced decision was to address this after the 1.4 release. That release is not yet complete. So much code, so little time.

@jonmeredith
Copy link
Contributor

But it can wait until after 1.4

On May 10, 2013, at 6:29 AM, Matthew Von-Maszewski notifications@github.com wrote:

Our unannounced decision was to address this after the 1.4 release. That release is not yet complete. So much code, so little time.


Reply to this email directly or view it on GitHub.

@matthewvon
Copy link
Contributor

8 months later ... try branch mv-iterator-prev ... unit testing passes, will start bulk testing next.

@matthewvon
Copy link
Contributor

bulk testing seems fine too. submitting a PR for the branch.

@bookshelfdave
Copy link
Contributor

@uwiger do you mind if I include your code from above as an eunit test?

@uwiger
Copy link
Author

uwiger commented Sep 5, 2013

Not at all - thanks for asking. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants