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

isEmbedded() expose what sort of contract the leveldown can provide. #279

Closed
dominictarr opened this issue Sep 12, 2014 · 33 comments
Closed
Labels
discussion Discussion enhancement New feature or request

Comments

@dominictarr
Copy link
Contributor

After seeing a talk at nodeconf eu that used sqldown I realized it would be impossible to stop people using levelup with non-embedded back ends, and having a discussion of this #255 scale every time would only turn me into a grumpy old bastard...

Then I realized there was a simpler possibility, we add an db.isEmbedded() method this would be exposed on the levelup and leveldown instances, and modules that do not support non-embedded databases can throw with a helpful error message (that links to documentation on the distinction between embedded and service databases)

Also, there would be documentation on isEmbedded() and it would be easy to do a few pull requests to add isEmbedded to the various leveldowns. So it would be simpler than reimplementing all of level #270 (this will take a long time, anyway) but still allow people using level in other ways, such as @nlf @adambrault @fritzy are doing.

@nlf
Copy link

nlf commented Sep 12, 2014

👍

Seems reasonable to me.

Something @fritzy had brought up in the past was adding some kind of feature detection to *down backends, so this could potentially lay the groundwork for future development in that direction as well.

@mcollina
Copy link
Member

As discussed at nodeconfeu, 👍 for me.

@juliangruber
Copy link
Member

+1 to the general idea, embedded is the wrong word though isn't it? couldn't a non-embedded db support all of abstract-leveldown?

@dominictarr
Copy link
Contributor Author

@juliangruber yes, but it wouldn't support the implicit assumption of embeddedness - that all incoming data passes through this instance.
So, live streams and prehooks (which are the basis of many modules) do not work in non-embedded databases.

@juliangruber
Copy link
Member

ah ok. agreed, this is good to know. thinking about future demands we could simplify by implementing a broader compatibility function, like:

db.has('embedded')
db.has('snapshots')
db.has('batches')

etc

@heapwolf
Copy link
Contributor

+1 to Julian's suggestion!

On Saturday, September 13, 2014, Julian Gruber notifications@github.com
wrote:

ah ok. agreed, this is good to know. thinking about future demands we
could simplify by implementing a broader compatibility function, like:

db.has('embedded')db.has('snapshots')db.has('batches')

etc


Reply to this email directly or view it on GitHub
#279 (comment).

Paolo Fragomeni
Founder, Here is How
http://hereishow.to

github.com/hij1nx
twitter.com/hij1nx

@dominictarr
Copy link
Contributor Author

Or just db.supports[feature] ? and then that feature should probably throw if you try and use it.

@juliangruber
Copy link
Member

yeah, the exact api is probably best discussed in a pr. but big +1 to the idea

@nolanlawson
Copy link
Contributor

+1 for Julian's idea. It would also be nice if the abstract-leveldown tests checked for such things before running certain tests, e.g. for the snapshot test.

@juliangruber
Copy link
Member

...and if we had a module that takes those compatibility results and generates a big badge for readmes and a small one for the levelup wiki

@nolanlawson
Copy link
Contributor

+1000

@calvinmetcalf
Copy link
Contributor

taking a step back what is the isEmbedded supposed to check for, that only one process can connect to it at once?

because sqlite3 is embedded, but it can have multiple processes have it open which work screw up the ability to use sublevel

@dominictarr
Copy link
Contributor Author

@calvinmetcalf good point... maybe it should be isSingleProcess ?
Although, that sounds like it could be true when only one process happens to be connected...

So we want to know if the backend is strictly single process... this is more important than being embedded, per se.

Although, this is often implied by being embedded...

@mcollina
Copy link
Member

isEmbedded looks nicer. But I get the point of isSingleProcess.

How about hasExclusiveAccess?

@chesles
Copy link

chesles commented Sep 18, 2014

Sounded like there was a lot of +1's for Julian's idea for .has:

db.has('exclusive-access')

Although a has method could be confused as checking if a key exists in the db. Maybe db.supports(feature)?

@dominictarr
Copy link
Contributor Author

I disapprove of a using a function because it implies the result might be dynamic - this should be static - is depends on the implementation of the database, which doesn't dynamically change. (I realize that I originally suggested a function - but in the pr I made a boolean property: Level/leveldown#128)

I agree with @chesles that supports is better than has.

But also, although there are other things we could put in the supports object, there isn't actually a need for any other case than exclusive...

@juliangruber
Copy link
Member

A: "hey, i heard there a different backends for levelup, some really crazy like s3 and mysql. don't they not support all the features?"
B: "yeah that's right. if you want to see exactly which features a backend supports, check out the badge on the readme, or log db.supports"

I'm for db.supports as an object.

@dominictarr
Copy link
Contributor Author

okay so what are the assurances that leveldb gives us that other backends might not?

I'm thinking exclusive access, snapshots (on iterators), and batches.

@juliangruber
Copy link
Member

binary keys / values

@juliangruber
Copy link
Member

unlimited size of keys / values

@juliangruber
Copy link
Member

backend implementors probably know best here

@dominictarr
Copy link
Contributor Author

I feel there is some danger in adding a bunch of things here that are currently hypothetical and don't represent a clear and present danger to some level-* module. (i.e. cause it's tests to fail)

@calvinmetcalf
Copy link
Contributor

  • permanence (e.g. memdown)
  • multi access (leveldown would be no, sqldown would be yes)
  • buffers (this is an annoying difference between some of the browser ones)
  • snapshots (an issue in the wild with memdown)

@juliangruber
Copy link
Member

those sound good.

i don't care too much which features we add, it's only important to me that we make this feature detection generic

@dominictarr
Copy link
Contributor Author

@calvinmetcalf can you be explicit about which backends and which modules are affected by a particular issue, such as buffers

Snapshots affect pull-level and level-live-stream if you read both old and new records in one stream.
Any module that uses prehooks to do indexing is affected by non-exclusive access.
Batch will break loads of modules - including the ones that use prehooks.

@calvinmetcalf
Copy link
Contributor

so multi access is something that effects backends, it either throws an error or doesn't, requiring single access is something that effects plugin-ins. sqldown can handle multi-access, any sort of sublevel based indexing requres sole access no mater what the backend.

buffers effects downs in the browser sometimes, they return strings instead of buffers, I assume these can be context dependent so levelup returns something about what the default return type is

@chesles
Copy link

chesles commented Sep 18, 2014

It sounds like we have 2 sets of things: features a particular backend supports, and features a particular plugin needs support for.

Building off the idea for a module to generate compatibility badges, if each plugin defined a
needs_support_for object (or array) we could generate a compatibility list for each plugin, so you could easily see what backends a plugin will work with. That would be pretty neat in a plugin readme or wiki.

@dominictarr
Copy link
Contributor Author

everything should be able to provide buffers, although it might be necessary to do some wacky polyfil.
@calvinmetcalf which client side *down doesn't do buffers right?

@calvinmetcalf
Copy link
Contributor

It's not that they don't do them right it's that they do them differently
so that the flag for default to buffer (or whatever) would very between
browsers and node
On Sep 18, 2014 7:26 PM, "Dominic Tarr" notifications@github.com wrote:

everything should be able to provide buffers, although it might be
necessary to do some wacky polyfil.
@calvinmetcalf https://github.com/calvinmetcalf which client side *down
doesn't do buffers right?


Reply to this email directly or view it on GitHub
#279 (comment).

@nolanlawson
Copy link
Contributor

@calvinmetcalf If it would be easier, we could just release a major version for localstoragedown/memdown and then return buffers instead of strings.

Admittedly, this is probably something we should add as an abstract-leveldown test, so that we could define it as correct behavior, instead of just fixing it in Pouch as we've lazily done up till now.

@rvagg
Copy link
Member

rvagg commented Oct 2, 2014

Next step here is for someone to PR against abstract-leveldown with a test case that (1) checks that db.supports exists and that typeof db.supports.foo, db.supports.bar, etc. is as expected and within acceptable bounds. Individual backends can then extend the tests to make sure they are returning exactly what they need.

We can bikeshed supports vs has etc. once we see what someone's PR looks like.

I'm +1 on limiting it to only what is needed by actual extensions at the moment, with exclusive being the obvious one. There needs to be a solid justification each time someone wants to add some new cruft into it.

@vweevers
Copy link
Member

Though some differences between memdown, level-js and leveldown have since been fixed (with regards to snapshots and buffers), there are lots of interesting ideas here that relate to manifests, so ref: Level/subleveldown#35 and Level/deferred-leveldown#35.

@vweevers
Copy link
Member

Closing in favor of Level/community#42 (which summarizes this thread).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

10 participants