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

Serialize compact range options #517

Merged
merged 6 commits into from
Nov 24, 2018
Merged

Conversation

vweevers
Copy link
Member

Closes #499.

The last commit (d54db39) (making start and end optional) started as an exercise, to see how the nullish/empty logic translates to compactRange. Answer: it's counterintuitive.

If, for symmetry with iterator range options, we cannot use a nullish start or end, then it only leaves '' as "not defined" signal. On the C++ LevelDB side, it is the other way around. We can do 5 things:

  1. Nothing, because having to do compactRange('', 'z', cb) was already the case, though compactRange(null, 'z', cb) also worked (undocumented) and now it doesn't.
  2. Revert d54db39, make it a problem for a later day
  3. Forgo symmetry with iterator range options, so that e.g. compactRange(null, 'z', cb) compacts first key to 'z' but iterator({ gte: null, lte: 'z' }) targets 'null' to 'z'.
  4. Have nullish mean "not defined" everywhere (Carefully consider behavior of nullish range options #515, comes with caveats), so that compactRange(null, 'z', cb) targets the same range as iterator({ gte: null, lte: 'z' }).
  5. Introduce options object so that consumers don't need null, e.g. compactRange({ end: 'z' }, cb).

db = testCommon.factory()
db.open(t.end.bind(t))
make('test compactRange() frees disk space after key deletion', function (db, t, done) {
db.batch().put(key1, val1).put(key2, val2).write(function (err) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

ralphtheninja
ralphtheninja previously approved these changes Oct 21, 2018
@ralphtheninja
Copy link
Member

Not sure what I prefer out of those options. I'm leaning towards 5.

@vweevers
Copy link
Member Author

It's too bad that LevelDB doesn't support approximateSize of the full database, otherwise we could refactor that to take an options object too.

@vweevers
Copy link
Member Author

Option 5 also makes it easier to introduce promise support in the future, i.e. making the callback optional. To do that while also making start and end optional would mean we'd have to detect the following call signatures:

compactRange(start, end, callback)
compactRange(start, callback)
compactRange(/* not-defined-signal */, end, callback)
compactRange(callback)
compactRange(start, end).then(..)
compactRange(start).then(..)
compactRange(/* not-defined-signal */, end).then(..)
compactRange().then(..)

Vs.

compactRange(options, callback)
compactRange(callback)
compactRange(options).then(..)
compactRange().then(..)

@vweevers
Copy link
Member Author

I'd choose 5 if:

  • approximateSize didn't exist or its arguments were optional in LevelDB
  • We didn't already have many breaking changes (from abstract-leveldown)
  • The C++ was cleaner. Because regardless of whether the public interface takes start and end arguments or an options argument, internally we'd need to default to '' and translate that to null on the C++ side. Using null on both sides would require more refactoring, to skip disposing slices. If anyone wants to take a stab at that though, go for it!

So at this time, I prefer option 2. That keeps the code simple (and easy to port to leveldown's friends):

leveldown/leveldown.js

Lines 78 to 83 in 0e3cda4

LevelDOWN.prototype.compactRange = function (start, end, callback) {
start = this._serializeKey(start)
end = this._serializeKey(end)
this.binding.compactRange(start, end, callback)
}

@vweevers vweevers changed the title [WIP] Serialize compact range options Serialize compact range options Nov 24, 2018
@vweevers
Copy link
Member Author

Squash on merge.

@vweevers vweevers merged commit c604cc4 into master Nov 24, 2018
@vweevers vweevers deleted the serialize-compact-range-options branch November 24, 2018 20:49
@rvagg rvagg mentioned this pull request Dec 18, 2018
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.

2 participants