Level / rocksdb

Pure C++ Node.js RocksDB binding. An abstract-leveldown compliant store.
MIT License
229 stars 53 forks source link

refactor, simplify and modernize code #197

Closed ronag closed 2 years ago

ronag commented 2 years ago

I realise that this is probably not a landable PR nor is it very review friendly. We are trying to switch over to RocksDB and are on a somewhat tight schedule so I've just done the cleanup and refactoring that we want and thought I'd at least make a PR for inspiration. If there are any part that is of interest I could try and open more split up PR's if and when I have time.

Some things I'm currently stuck on:

vweevers commented 2 years ago

I realise that this is probably not a landable PR

Yeah. The way forward for rocksdb is to fork it and move to abstract-level (tracked by https://github.com/Level/abstract-level/issues/14, I haven't started on it yet).

In addition - but this might change with your involvement - rocksdb has had few npm downloads, few contributors, few maintainers, so for the past years, the approach has been to cherry-pick patches from leveldown (or soon classic-level), keeping the code the same (except for a few differences in options and test assertions) to make that process quick and easy.

vweevers commented 2 years ago

I haven't started on it yet

If you or someone else wants to tackle that sooner, LMK, I can write down the steps that I plan to take

ronag commented 2 years ago

I think RocksDB will be more interesting for us than LevelDB. I'm considering also spending some time moving this over to abstract-level. However, I still get better performance with the old API since the iterator API can use a flat array instead of an entries (array of arrays) array. Maybe we could discuss improving this?

I'd be interested in contributing more on this repo. But I would maybe need some help with setting up/updating the native dependencies.

vweevers commented 2 years ago

Correction after looking at my local repo's - I did already start on the move to abstract-level 😄 I'll have a closer look later (end of week or next week) to see in what state I left that. Could be a good starting point for further improvements (but we may want to land your classic-level PRs first, port those over, and then diverge from there on).

I'd be interested in contributing more on this repo.

Glad to hear! I'll write more once I have time

ronag commented 2 years ago

TBH: I'd replace the backend for classic-level to rocksdb. Most users wouldn't notice any other difference than improved perf.

vweevers commented 2 years ago

That's definitely not an option. RocksDB is not suited for embedded use, it's bigger, a pain to build, and overkill for the majority of use cases.

ronag commented 2 years ago

@vweevers I've fully ported to abstract-level.

ronag commented 2 years ago

RocksDB is not suited for embedded use, it's bigger, a pain to build, and overkill for the majority of use cases.

Are you aware there is a "lite" option? Not sure what it is and whether or not it's relevant.

jacoscaz commented 1 year ago

Is the port to abstract-level still ongoing and/or available somewhere @ronag ?