Snapshot test - do we really need it?

[nolanlawson]

I noticed this test was added recently. It's causing some headaches for me in localstorage-down, but I'm wondering how many other *down authors have actually implemented this thing?

Reading the test, I can understand the problem it's trying to solve. Ideally users should be able to open up a read-only iterator against a database and continue to read from a "snapshot" even as others are writing to it.

However, that's a really, really involved feature (concurrency! transactions!), especially for simple modules like MemDOWN and localstorage-down. And this test doesn't even seem to be very thorough. I could adhere to the letter of the law by just passing this one test, while still not really implementing the proper transactional semantics.

What's the feeling about this? Should *down authors pick-and-choose the tests we know we won't support, or should we rise to the occasion and try to pass tests like this?

[rvagg]

My initial thoughts on this were that it's likely going to be a leveldb-specific thing with other *downs deciding either way whether they can do it. It's probably important that implementers document if they can't do it because that may impact on the extension possibilities. There will be a few cases where it'll be very important that it works but those relying on it will have to understand whether it's available to them or not.

Once we start exposing explicit snapshots in leveldown then it'll become an even more obvious distinction. The fact that you'll be able to pass a snapshot object to createReadStream() should make it clear to the user what's going on and they'll have to understand if it's going to work or not (perhaps the default *down behaviour should be to throw when it gets a 'snapshot' property on the options even?).

[rvagg]

@kesla @dominictarr your thoughts on this would be helpful too I think

[kesla]

I wrote the test as a complement to Level/leveldown@76e93c4 and some stuff I did i medeadown - I saw medeadown & leveldown behaving differently and got confused about it.

Other than that I mostly agree what @rvagg wrote, it's important for implementors to clarify if they support this feature or not.
So, @nolanlawson in your case I'd say that one solution would be to simply omit the snapshot-tests from the tests that you're supporting.

[nolanlawson]

Thanks for the clarification. I think that's likely the direction I'm going to go in – I'll just omit the snapshot tests and then clarify that localstorage-down doesn't support it.

Having some kind of AbstractSnapshotter that we could share in both MemDOWN and localstorage-down would be awesome, though. ;)

[hmalphettes]

I had to do the same in redisdown: hmalphettes/redisdown@8009b23
and clarified in the Readme that this is not supported: "Abstract-LevelDOWN testsuite is green except for the 'implicit iterator snapshot'."

Is there a well agreed way to present this limitation?
Should we come up with some documentation that we could refer to to present the limitation.

In the case of redisdown it is creating a fair amount of uncertainty: hmalphettes/redisdown#10 (comment)

[rvagg]

Snapshots will be important for some extension use-cases so it'll be worth documenting the implications somewhere. Unfortunately it's not a simple one-line explanation of why they are important. In reality it'll either be very difficult, or impossible to implement in most backends, which is unfortunate but this is a fact that anybody relying on the feature will have to take into account as well.

Perhaps we need to compile a list of use-cases where implicit snapshots for iterators are important and then write up a post somewhere about it. Anybody want to throw in suggestions?

/cc @dominictarr & @mikeal who have probably put some thought into this topic already

[mikeal]

adding @maxogden who has talked about using stuff like this for dat.

[nolanlawson]

I would also like us to figure out a sane versioning scheme for this module.

The fact that this snapshot test was introduced as a patch release, thus breaking both memdown (Level/memdown#21) and localstorage-down and who knows how many other *down modules, is a huge nuisance.

I feel like maybe everything should be a major version change for abstract-localstoragedown.

[rvagg]

@nolanlawson sorry, that would be my fault, shouldn't have been a patch release, we should be sticking to proper semver(ish). Minor versions are fine, just don't use ^ in your dependencies if you really want to protect against accidentally killing kittens.

[timkuijsten]

While implementing IndexedDB for Level.js I've encountered the problem that, although idb cursors support snapshotting, browsers time them out aggressively, meaning that if _next isn't called within the next tick, there is a high probability that the cursor timed out. I've added a reopenOnTimeout option to the iterator, but this defeats the snapshot property. Also, the current set of abstract tests are not "fast enough" to prevent timeouts, as noted in Level/level-js#46 (comment). I'm wondering if I should change the default of reopenOnTimeout to true, a.k.a. non-snapshot mode. Then all tests pass (even the abstract snapshot tests since they are fast enough) but this differs from the LevelDB defaults and what a user might expect.

[vweevers]

In v6 you will be able to skip the snapshot test. At the time of writing, usage is:

require('abstract-leveldown/test')({
  test: require('tape'),
  factory: function () {
   return new MyAbstractLevelDOWN()
  },
  snapshots: false
})