Bite the bullet and remove WriteStream completely?

[rvagg]

WriteStream is only there for the sake of symmetry but that's not a great reason in itself if there are better reasons for it not to be there. I think we're starting to realise that there are different use-cases that require different implementations and we're already seeing some alternative implementations so why not set them free to coexist and compete in user-land?

Discuss

If we conclude that it should be removed then we can add documentation to the ReadStream section saying that if you want a WriteStream then go to a particular place on the wiki to see a list of implementations and what they are good for. We could even dedicate a whole wiki page to this if need be.

if we concluded that it should stay, then I'll try and get these competing pull requests for new WriteStreams reconciled and sort something out because having a streams2 ReadStream and a streams1 WriteStream is a mess and this needs to be resolved.

[heapwolf]

+1 for moving it into userland! levelup should only maintain for common cases. do like we want with node core, expose the critical primitives and let userland do the bike-shedding.

[Raynos]

👍 for user land as well.

My userland precursor to level.js (levelup on top of indexeddb https://github.com/Raynos/levelidb/blob/master/stream.js#L2) also had write-stream as a user land module rather then being implemented as a core primitive.

[brycebaril]

Hmm, I'm less enthusiastic about moving it to userland. I've used WriteStream enough to make me worry that a userland implementation would fracture support when combining level-* modules. For example, level-version wraps WriteStream with special behavior to handle the additional version metadata. Having WriteStream in the core (and thus wrapped by level-version) means that level-gc can assume that level-version handles anything version-related with WriteStream.

The ReadStream/WriteStream symmetry I think is important for modules given it is the most logical way to do things like replication, which to me feels like an important common use-case.

It feels levelup core to me.

[heapwolf]

@brycebaril I think you make good/fair points, except i'd say that streaming APIs aren't necessarily coupled with replication schemes.

[rvagg]

@brycebaril what's stopping you from wrapping the actual primitives that any WriteStream implementation will have to use? You're still in a situation where you may have to deal with alternative WriteStream implementations as people may have data that's suited to a different mechanism, like @maxogden is finding out. If you can wrap & extend only the primitives then you have the most portable solution in any case.

[rvagg]

Having fewer "primtives" to deal with ought to be a bonus for libraries extending levelup, WriteStream is a false primitive because it just builds on the write operations (put, del, batch).

[brycebaril]

No, that's fine -- any sufficient levelup wrapper should fully wrap the put/del/batch primitives, so I guess any WriteStream library that correctly wrapped the primitives should plug into anything else.

I'm still in favor of keeping it for the symmetry, but I guess I haven't fully kept on top of why the WriteStream implementations differ in ways that make them more/less suitable for different use-cases. Is there a summary somewhere I can catch up on?

I'm not sure that I can get behind the 'less work for an extending library' argument unless we can also do something like get rid of one of the two batch implementations. Each of those requires significantly more work than wrapping WriteStream.

So far it looks like I'm the lone supporter of WriteStream, and I can certainly live without it, though one of the things that I feel sells this database is the native Streams support. I can't help but feel removing streaming features will chip away at that veneer of magic.

[max-mapper]

One the one ✋ I agree with @brycebaril about the read/write API parity being convenient, on the other ✋ I don't think whatever writestream implementation gets included will ever be the fastest possible way to write data into leveldb for all use cases.

Basically, if you are using leveldown, and you wanna optimize write throughput, you wanna write data in batches no larger than the leveldb writeBufferSize. The logic for doing that is sufficiently complex to warrant it's module (byte-stream), so including byte-stream or something like it in levelup wouldn't really make sense as the approach there might not make any sense at all with other backends.

So in the case of using leveldown you will end up using a third party module for write streams anyway because it will be faster, which begs the question "why have a write stream in levelup at all?".

So it kind of boils down to keeping them in for convenience/API parity with read streams, which means that its that much more API surface area to support in levelup which means that people might ask questions like 'why am I only getting 2000 inserts per second when the leveldb benchmarks say I should be getting 100,000?' because they used the write stream that came with levelup but didn't realize they needed to install a third party module that fit their use case.

These kinds of API decisions are hard to make because you have to anticipate what complex effects they will have down the road. My gut feeling would be to err on the side of limited API, but I'm not really 100% confident either way on this issue.

[alexindigo]

And you can keep level module (bundle) as user- (consumer-) friendly alternative for regular peps, as plug-n-play solution and example of how modules work together.

[mcollina]

I'm definitely 👍 on this.

First, it reduces API area. Less stuff to maintain!
Second, you end up having a different implementation anyway.

I'm with @alexindigo that we should 'bless' an implementation and add it to the level module.

I think we can deprecate it in the next release, and then remove it in a month or so.

[chilts]

Sounds like there is unanimous agreement. Removing it means less surface area, fewer primitives to wrap and less to maintain in the current code. :)

[pgte]

I'm also +1 on this one. From the benchmarks we saw that optimizing for different use cases is nearly impossible, so the writestream needs to be pushed into userland since any default "optimization" will most probably be wrong.

Also, this opens up even more the debate and documentation of write performance, which is always good.

[mcollina]

I think we should mantain some kind of high-level tests for a writestream to make them interchangeable.

[rvagg]

We could maintain a reference implementation that does the simplest thing, like @substack's PR does now, it could live in a repo under the level org and have a set of reference tests that other WriteStream implementers could use as a foundation for their test suites, like we do with abstract-leveldown.

[dominictarr]

Hmm, so with replication modules, I've usually needed more exact schemantics,
so I've implemented my own write-stream that enforces things like updates a sequence number in a controlled way.

I think this is really about bulk loads. We need more research in this area, and test cases.
@maxogden you have the most experience here, can you explain, or link us to the techniques needed?

This is something that could benefit from test cases that could be reused across different writestream implementations.

I'm in favor of this in principle, but I think we should hold off until we know a bit more about the problems.