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

plugin injection points #48

Open
dominictarr opened this issue Apr 20, 2013 · 24 comments
Open

plugin injection points #48

dominictarr opened this issue Apr 20, 2013 · 24 comments
Labels
discussion Discussion

Comments

@dominictarr
Copy link
Contributor

So, we have had quite a bit of discussion, and tried various approaches to implementing plugins in levelup

https://github.com/rvagg/node-levelup/issues/search?q=plugins

quick summation of what has happened so far, I started experimenting with a crude monkey patching based approach, but ran into trouble with handling ranges - each plugin had manage which ranges it affected, which was tricky. I later refactored this to create a subsection of the database, with level-sublevel. This is a great improvement, because it allows you to extend a range within leveldb as if it's a whole db.

@rvagg has also experimented with exposing various integration points into levelup, https://github.com/rvagg/node-levelup/issues/92

personally, I am highly in favor of combining these two, and even merging sublevel into levelup, or at least, adding integration points to levelup so that level-sublevel does not have to monkey patch it.

the question is: what is the list of integration points that we need?

  • prehooks (intercept a mutation [batch, put, del])
  • posthooks (intercept a mutation callback)
  • encoding/key-encoding
  • setup (register special jobs that run first, after the database opens,
  • asynchronously delay mutations. **

** maybe. The ability to get the current values for keys before performing a mutation.
this would be useful for validation, and merging concurrent updates.

I have a plugin for this, but it hasn't been updated to work with level-sublevel yet. This differs from level-hooks, which only provides
a sync api.

A setup integration point will be useful for saving metadata about the database in the database, and maybe stuff like summaries about the current overall state - whether a schema change migration is complete, etc.

Any other suggestions?

@rvagg
Copy link
Member

rvagg commented Apr 20, 2013

On my wishlist is an easier way to interface with the main methods so I don't have to decode arguments like LevelUP does. So I can interact with the API: db.put(key, value[, options][, callback]) yet have something predictable like: db.put(key, value, options, callback), where options is an object with the default options + any user supplied options and even having an actual callback without having to check that it exists and is a function. The options are particularly awkward because you can pass nothing, a String (= valueEncoding) or an object, then LevelUP takes the options on the current instance and extends them with what you've provided to obtain the options for the current method call. I don't want to have to do all that in my plugins just to replicate the LevelUP API. That's one thing I was trying to achieve in #92.

Also, FTR, I'm open to level-hooks and possibly level-sublevel integration in core if necessary because I've come around to seeing what a useful abstraction it is and it could be made even more useful if it had better access in to core.

@ralphtheninja
Copy link
Member

+1 for level-sublevel and hooks in core, they are the sugar that makes this
db just a little more than "just" a key/value store, I also firmly believe
that many will have use of this.

On 20 April 2013 03:11, Rod Vagg [email protected] wrote:

On my wishlist is an easier way to interface with the main methods so I
don't have to decode arguments like LevelUP does. So I can interact with
the API: db.put(key, value[, options][, callback]) yet have something
predictable like: db.put(key, value, options, callback), where options is
an object with the default options + any user supplied options and even
having an actual callback without having to check that it exists and is
a function. The options are particularly awkward because you can pass
nothing, a String (= valueEncoding) or an object, then LevelUP takes the
options on the current instance and extends them with what you've provided
to obtain the options for the current method call. I don't want to have
to do all that in my plugins just to replicate the LevelUP API. That's one
thing I was trying to achieve in #92https://github.com/rvagg/node-levelup/issues/92
.

Also, FTR, I'm open to level-hooks and possibly level-sublevel integration
in core if necessary because I've come around to seeing what a useful
abstraction it is and it could be made even more useful if it had better
access in to core.


Reply to this email directly or view it on GitHubhttps://github.com/rvagg/node-levelup/issues/123#issuecomment-16696488
.

@navaru
Copy link

navaru commented Apr 20, 2013

+1 for level-sublevel and hooks in core

@dominictarr
Copy link
Contributor Author

Okay, cool. @rvagg I agree, that would also make the implementation of sublevel much cleaner.
One thing to add to this: if you want to, say, log all the changes in a database, you have to hook into anything that
can modify the database, so you hook put, and you hook del, and batches have both put and delete, so you have to hook batch.

In level hooks, I funnel these all together and call the hook with {key: key, value: value, type: 'put' || 'del'}
this api works whether it was a put, del, or batch.

So, maybe, we could make each method just call another method: _mutate, which can be either a put, del, or a batch.
this would call any integration points, then return the object to be inserted.

Sometimes, you want to turn a put/del into a batch - this is used in level-trigger (which is part of map-reduce) and level-master. Of course, if you add another op into a different range that needs to trigger hooks too. (adding it into the same range would probably make a infinite loop - this throws early in recent level-sublevel)

Perhaps mutate could cb async? Or, it could be called in the context of a batch object? (overhead? - or pooled?)

@rvagg did you intend the put/del/batch integration points to be async?

Another thing that hooks does, is allow you to register a range for the hook.
Like an event emitter allows you to listen on a single event.
Hooks allows you to do: db.pre({min: a, max: z}, function (op, add) {...})

Hmm, I'm feeling like I need to do a write-up of exactly how level-sublevel works...

@heapwolf
Copy link
Contributor

I'd be way more likely to add sublevel support to levelweb and lev if it was in core. I was already close to adding it, but this would finalize my decision. +1

@juliangruber
Copy link
Member

It's like with node and core modules. The http module could live on npm too, but it's so core-y and heavily used that the core is the place it belongs.

Since LevelUP is not only about accessing a LevelDB but soo much more about the idea of writing databases by layering abstractions through simple modules, extendability should be in core. And sublevel is there to help.

@rvagg
Copy link
Member

rvagg commented Apr 22, 2013

I have mixed feelings about bundling stuff in to LevelUP, a lot of us think that Node core is too large as it is now and we don't want LevelUP getting too out of hand. At least we have LevelDOWN available as a perfectly usable and safe interface though.

Re async hooks, I think that's a must and since the calls down to LevelDB are async anyway it's not really going to impact on the external API in any way. I did some basic benchmarking of the stuff I have on the branch I talked about in #92 with externr wired up and the impact was minimal even when you're funnelling everything through an additional layer of calls. V8 seems to be able to optimise effective noops away, so if you're not using anything in the plugin/hook layer then we ought to be able to get it to perform close to the current versinon.

One thing I'd like to register while I'm thinking of it is that if we ended up bringing in sublevel to core (lets deal with hooks first, maybe sublevel after that), we have to make sure that if people don't want to use it then it doesn't get in the way. Currently it'll mangle your ranges so as to ignore anything /^\xff.*/ but that assumes you're using that as a special namespaced area, which you may not be if you're not into the whole sublevel approach and you're just using LevelUP to pursue something completely different. i.e. a db.createReadStream() on the root database object should return the full database unless you've opted out (perhaps just creating a sublevel is enough of an "opt-out").

Aside from that, I'm confident that @dominictarr has thought through this enough and discussed it enough on IRC to come up with a decent approach to start us off with hooks integration. Plus we have a good collection of LevelUP add-ons now to use as examples of the kinds of things we want to be able to do. Unfortunately I don't have a whole lot of time right now to spend reflecting on the best way to do this and not make a mess of it.

@juliangruber
Copy link
Member

we have levelDOWN, levelUp and recently level too! This means levelUp needn't necessarily know about sublevel, it can all be in level.

@rvagg
Copy link
Member

rvagg commented Apr 22, 2013

Yeah, that's true, but we still need better hooks integration to make it all betterer.

@dominictarr
Copy link
Contributor Author

I also have reservations about pulling all of level-sublevel into levelup,
but if we can take the most generic building blocks from sublevel into levelup.
Mainly this is hooks on key ranges.

Also whole heartedly agree that not using any fancy features should not affect the benchmarks!

@jpillora
Copy link

+1 to thin core
Disclaimer: I'm a LevelUP/DOWN newbie.
Just listening to the NodeUp podcast, seeing as you're all here...
What about using the plugin style connect uses

So:

var LevelUp = require('levelup');
var Sublevel = require('level-sublevel');
var MyPlugin = require('./my-plugin');

var db = LevelUp('/tmp/sublevel-example');

db.use(Sublevel);
db.use(MyPlugin);

db.put('key', ...);
// will first hit SubLevel
// then MyPlugin

All plugins could be in the format:

exports.put(key, value, next) { ... }
//next instead of callback as it's actually just moving down the plugin stack
exports.get(key, next) { ... }
// no 'del' implementation
// and then you could leave implementations out and missing ones would
// be inherited from the prior plugin (this would also mean the plugin in the
// middle of the stack could add a foo() function that the rise to the surface)

Another idea:

db.use(Sublevel, 'arg1', 'arg2');
// use could accept plugin constructor args 

Another idea:

var db1 = db.extend(Sublevel, 'one');
var db2 = db.extend(Sublevel, 'two');
// could also extend instead of use, to keep the original db intact 

Another idea, incase its better to extend the class, rather than the instance:

var LevelUp = require('levelup');
var Sublevel = LevelUp.extend(require('level-sublevel'));
//or
LevelUp.use(require('level-sublevel'));

This may all be silly as I don't really know about how LevelUP/DOWN works internally...

@jpillora
Copy link

Also to address @dominictarr s points:

  • prehooks (intercept a mutation [batch, put, del])
    • use higher up the plugin stack
  • posthooks (intercept a mutation callback)
    • use lower down the plugin stack
  • encoding/key-encoding
    • use(JSONEncoder)
  • setup (register special jobs that run first, after the database opens,
    • plugin constructors as mentioned above, and for async, maybe levelup could provide a callback ?
  • asynchronously delay mutations.
    • not sure what this is...

@juliangruber
Copy link
Member

it basically has

var req= {
  key: key,
  val: val,
  method: method /* put, del, etc. */
}
var res = {
  end: function (err, data) { /* finally save */ }
}

@jpillora
Copy link

Right, but instead passing all data through one function and saying method === 'put', wouldn't it be faster to call an exported put function?

@juliangruber
Copy link
Member

how would you change the key for all operations?

exports.put = function (key, value, next) {
  key = 'foo'; // now you reassigned `key` instead of changing it
  next();
}

that would be fixed by passing an object:

exports.put = function (kv, next) {
  kv.key = 'foo';
  next();
}

this would be more performant than my api, I agree. But why just limit the performance increase to filtering out methods, we could also filter keys:

function plugin (db) {
  db.intersect({
    method: 'put',
    key: { start: 'aaa', end: 'ccc' },
    /* or */
    key: /[a-c]{3}/,
    fn: function (kv, next) { /* ... */ }
  })
}

@jpillora
Copy link

What about:

exports.put = function (key, value, next) {
  key = 'foo';
  next(key, value);
}

Where the params for next would mirror the args of the function, minus next

@jpillora
Copy link

Maybe a special case for error ? using:

next(new Error(...));

Or maintain node style and:

next(null, key, value);

@rvagg
Copy link
Member

rvagg commented Jul 25, 2013

this is roughly similar to what I have in #92 using externr; but I'm just not sure... tbh aside from not having the time to think this through clearly enough I'm really hoping that @dominictarr will come up with some brilliant abstraction that solves all our problems, but that might be a little unrealistic of me!

@jpillora
Copy link

@rvagg ah quite similar indeed. Though by going with the connect style, plugin precedence will be much clearer, and also it (kind-of) places the monkey patching into the hands of the user:

var MyPlugin = require('./my-plugin');
var db = LevelUp('/tmp/db');
db.use(MyPlugin);
db.put('key', ...);

As opposed to:

require('./my-plugin').use()
var db = LevelUp('/tmp/db');
db.put('key', ...);

Also, in the former example, the monkey patching is only applied to one specific db instance, instead of the whole LevelUp module.

@rvagg
Copy link
Member

rvagg commented Jul 25, 2013

yeah, that's one thing I'd change about it now if I was to redo that pull request

@jpillora
Copy link

Could still be redone

Major version zero (0.y.z) is for initial development. Anything may change at any time. The public API should not be considered stable.
http://semver.org/

😉

@rvagg
Copy link
Member

rvagg commented Jul 25, 2013

The point is that we're still searching for an acceptable abstraction here that serves the growing list of use-cases and gets past the need for the endless monkey-patching that we're all doing. I haven't redone that PR because I'm not overly committed to the approach (and neither was anyone else as it turned out!).
I'm not convinced the connect-style is the way to go but you're welcome (and invited!) to experiment. There's a ton of existing use-cases that you can look through in the modules list to see the kind of things people need to do.

@jpillora
Copy link

Fair enough, I haven't been through all the use-cases yet and the connect-style of plugins may be inadequate, though when I find some time, I'll temper my idea with some research (only problem is the time finding >.<). Currently trying to make a case for leveldb at work...

@ralphtheninja ralphtheninja reopened this Dec 18, 2018
@ralphtheninja ralphtheninja transferred this issue from Level/levelup Dec 18, 2018
@vweevers vweevers added the discussion Discussion label Jan 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion
Projects
Status: Backlog
Development

No branches or pull requests

8 participants