Flag to disable iterator tests

[piranna]

abstract-leveldown/abstract-leveldown.js

Lines 182 to 184 in 9d107d0

	AbstractLevelDOWN.prototype._batch = function (array, options, callback) {
	process.nextTick(callback)
	}

Default _batch implementation is a no-op, forcing you to implement it yourself to make abstract-level/test suite in your own AbstractLevel object. I think it would make sense to have a default implementation that just run over the given operations calling the object methods, something like

  _batch(operations, options, callback)
  {
    async.each(operations, ({key, type, value}, callback) =>
    {
      if(type === 'put') return this.put(key, value, options, callback)

      this[type](key, options, callback)
    },
    callback)
  }

Main issue would be to add rollback support if one of the operations failed, but maybe this can be left as task for the reader since it's a more advanced and probably underlying backend dependent :-) Thoughs? Would you accept a pull-request for that?

[vweevers]

It's hard to make a default implementation atomic.

[piranna]

I know, that's why I said about leaving the atomic implementation up to the reader :-) Just only provide a "just enough" default one.

[vweevers]

IMO if the default can't be atomic, and therefor cannot pass the test suite (I don't remember if this is actually covered by the test suite), we shouldn't have a default.

[piranna]

In that case, maybe a good alternative would be to add a flag to disable batch tests, so it's another way to say "this AbstractLevel doesn't support atomic batch operations", what do you think?

Oh, and said that, maybe also this kind of informative test flags could be set as a metadata property of the AbstractLevel object itself that can be queried in runtime too, but this should be addresed better in a diferent issue :-)

[vweevers]

In that case, maybe a good alternative would be to add a flag to disable batch tests, so it's another way to say "this AbstractLevel doesn't support atomic batch operations", what do you think?

I'm not opposed to adding a flag per se (in addition to the ones we already have), but only if there's an abstract-leveldown implementation that can't make batches atomic with good reason.

If we were to add a default implementation of batch() that isn't atomic, the default value for the flag would have to be false. Which I don't like. The test suite and its default options should test for full compliance.

Oh, and said that, maybe also this kind of informative test flags could be set as a metadata property of the AbstractLevel object itself that can be queried in runtime too, but this should be addresed better in a diferent issue :-)

Yeah, that's been on our todo list for some time now (Level/community#42).

[piranna]

I'm not opposed to adding a flag per se (in addition to the ones we already have), but only if there's an abstract-leveldown implementation that can't make batches atomic with good reason.

Not so much about non-atomic batch operations, BUT not suporting batch operations at all. Not sure how much important is support of batch operations vs doing them one after one, I find it more like a performance improvement than a basic feature...

If we were to add a default implementation of batch() that isn't atomic, the default value for the flag would have to be false. Which I don't like. The test suite and its default options should test for full compliance.

Agree on that.

Yeah, that's been on our todo list for some time now (Level/community#42).

Great! :-D

[vweevers]

Not so much about non-atomic batch operations, BUT not suporting batch operations at all. Not sure how much important is support of batch operations vs doing them one after one, I find it more like a performance improvement than a basic feature

Oh I see. I do consider it a basic feature though. Writing multiple keys at once is a very common task in my experience. For convenience, performance, the atomicity of it (for example, to update indexes) and as a building block for writable streams.

[vweevers]

In addition, many modules in the ecosystem expect batch() to exist, like levelup, write streams, indexers, map-reduce utilities, etc.

[piranna]

I see... Ok, my level is a wrapper one, so in that case, if there's a warranty that the underlying has atomic batch support, maybe I can be able to provide it myself on top of that, instead as a sucesion of operations...

[vweevers]

if there's a warranty that the underlying has atomic batch support

That is (or should be) safe to assume.

[vweevers]

If it helps, here's how encoding-down wraps its underlying db: https://github.com/Level/encoding-down/blob/e5c0253dcc40f43d02928337c8fb1bcf868f1e5f/index.js#L69-L72

[piranna]

That is (or should be) safe to assume.

Cool :-) Maybe could be good to add it to docs? Honestly, I though the batch support was optional... In that way, I though del was optional and get could it be too (not my case, but would make sense to have a non-deletable or a read-only level...).

[piranna]

If it helps, here's how encoding-down wraps its underlying db: Level/encoding-down:index.js@e5c0253#L69-L72

Sort of what I was thinking about to do, thanks :-)

[vweevers]

=> #346

[piranna]

=> #346

Great, thank you :-)

[piranna]

Oh I see. I do consider it a basic feature though. Writing multiple keys at once is a very common task in my experience. For convenience, performance, the atomicity of it (for example, to update indexes) and as a building block for writable streams.

Are iterators a basic feature here too? Tests suite is failing me due to them now... What if it doesn't make sense or is not posible to iterate, like resources in a REST API where there's no list of all of them?

[vweevers]

Are iterators a basic feature here too?

Even more so. I'd say that iterators are Level's defining feature, that sets it apart from other key-value stores.

[piranna]

Even more so. I'd say that iterators are Level's defining feature, that sets it apart from other key-value stores.

🤦‍♂️ I just only wanted a key-value store... 😅 Ok, I'll try to implement it, I didn't want because I'm writing a SecureStore where keys are hashes, so no need to iterate over them.

[piranna]

I have tried to implement iterators, and got to the conclusion it doesn't makes sense at all in my level. In it, you could be able to seek to a particular entry using its key and calculating its key hash the same way you would access with get, put or del, and maybe you could be able to decipher the entry value from the key, but when moving to the next entry, you don't know its key because its stores as a hash in the underlying level (and hashes by definition are unidirectional), nor the value because its ciphered, so implement an iterator would be only just to return random garbage data. Not sure if this not qualify my level as an AbstractLevel, but think it's a valid use case to be able to add a flag to disable the iterator tests... Maybe a better definition would be "full" AbstractLevel compatible and "partial" AbstractLevel compatible levels? This could make even more sense when we have Level/community#42 available :-)