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

Use abstract-leveldown #50

Closed
wants to merge 16 commits into from
Closed

Use abstract-leveldown #50

wants to merge 16 commits into from

Conversation

kesla
Copy link
Contributor

@kesla kesla commented Aug 4, 2013

A WIP to fix #36, based upon #48 for now - so that one should probably be merged first.

@juliangruber
Copy link
Member

that looks very good to me!

@rvagg
Copy link
Member

rvagg commented Aug 4, 2013

yes yes yes

working on getting another NAN release out, might try and get that sorted today

will be interesting to see if we can measure any perf gain by shifting some of the code into V8 optimisable code.

@rvagg
Copy link
Member

rvagg commented Aug 5, 2013

updated to latest nan release, works for 0.8->0.11. BUT, there's an odd test failure that seems to happen every test run in 0.8, occasional test runs in 0.10 and never (I think) in 0.11:

not ok test/batch-test.js ............................. 23/24
    Command: "node" "batch-test.js"
    TAP version 13
    ok 1 cleanup returned an error
    ok 2 callback-less, 2-arg batch() throws
    ok 3 correct error message
    ok 4 correct error message
    ok 5 correct error message
    ok 6 correct error message
    ok 7 correct error message
    ok 8 no error
    ok 9 no error
    ok 10 no error
    ok 11 (unnamed assert)
    ok 12 should be equal
    ok 13 no error
    ok 14 no error
    ok 15 (unnamed assert)
    ok 16 should be equal
    ok 17 entry not found
    ok 18 value not returned
    not ok 19 NotFound error
      ---
        stack:  
          - getCaller (/home/rvagg/git/node-leveldown/node_modules/tap/lib/tap-assert.js:418:17)
          - Function.assert (/home/rvagg/git/node-leveldown/node_modules/tap/lib/tap-assert.js:21:16)
          - Test._testAssert [as ok] (/home/rvagg/git/node-leveldown/node_modules/tap/lib/tap-test.js:87:16)
          - /home/rvagg/git/node-leveldown/node_modules/abstract-leveldown/abstract/batch-test.js:111:11
        file:   /home/rvagg/git/node-leveldown/node_modules/tap/lib/tap-test.js
        line:   87
        column: 16
      ...
    ok 20 no error
    ok 21 (unnamed assert)
    ok 22 should be equal
    ok 23 cleanup returned an error
    ok 24 test/batch-test.js

    1..24
    # tests 24
    # pass  23
    # fail  1

I haven't looked in to it but we probably need to address it before a merge/release

@kesla
Copy link
Contributor Author

kesla commented Aug 5, 2013

I see the same test failing in #48, so I'd guess it's unrelated to this.

@kesla kesla mentioned this pull request Aug 5, 2013
@kesla
Copy link
Contributor Author

kesla commented Aug 5, 2013

Current state is that most things works.

I'm having problems with the iterator - the iterator-test sometimes just freezes after a little bit, @rvagg if you have the time and interest it would be very valuable to get your input on what might be causing that.

@rvagg
Copy link
Member

rvagg commented Aug 5, 2013

Will do, possibly related to the awkwardness of tracking and cleaning up unclosed iterators.

I'm busy getting ready for campjs at the moment, I'll either have time while I'm campjs this weekend or shortly thereafter.

@kesla
Copy link
Contributor Author

kesla commented Aug 5, 2013

I didn't change any of the code regardings tracking and cleaning up unclosed iterators, so that's all being done in C++-land still. I wanted to get your feedback before trying to move that to JS (and getting the other stuff working).

@kesla
Copy link
Contributor Author

kesla commented Aug 10, 2013

Btw, is there any special reasons that cleanup-hanging-iterators-test isn't part of abstract-leveldown?

@rvagg
Copy link
Member

rvagg commented Aug 15, 2013

cleanup-hanging-iterators could be part of abstract-leveldown I suppose, I'm trying to think of a reason why not but I guess it's something that may apply to other implementations.

@rvagg
Copy link
Member

rvagg commented Aug 15, 2013

so, this looks pretty good, there's an issue of style--a couple of things stand out, I'd prefer to see the functions declared straight on the prototype rather than as variables and then applied this might be contradictory to my older style but it's more clear and it's consistent across the level* projects (see AbstractLevelDOWN in particular). We also need to undo some of the private constructor stuff in LevelUP and do a similar thing there. And a minor nit is a space after function and before the parens makes it consistent with the rest of level*.

@rvagg
Copy link
Member

rvagg commented Aug 15, 2013

Lastly, thoughts on cleaning up unclosed iterators in JS vs C++, I don't mine either way, if you want to move it to JS then it'd certainly be cleaner. I'm fairly confident that the tests will catch you out if you do it wrong so just hack at it and see if you can make it work.

@kesla
Copy link
Contributor Author

kesla commented Aug 15, 2013

Cool, I haven't had time to look at this for a couple of days, but will hack on it during the weekend so I'll try and do the last parts then. Including the style fixes.

dominictarr pushed a commit to dominictarr/node-leveldown that referenced this pull request Aug 15, 2013
feature: Support setting size of LRU-cache
@kesla
Copy link
Contributor Author

kesla commented May 12, 2014

replaced by #97

@kesla kesla closed this May 12, 2014
@kesla kesla deleted the less-c++ branch May 12, 2014 22:12
@kesla kesla mentioned this pull request May 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let leveldown use abstract-leveldown
3 participants