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

refactor, simplify and modernize code #197

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link

@ronag ronag commented Apr 18, 2022

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:

  • Update rocksdb dependency to 7.1.x
  • Add zstd compression.
  • Add support for rocksdb columns as a more efficient implementation for sublevel.
  • Consider adding Ribbon filter support as an option.

@vweevers
Copy link
Member

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 Level/abstract-level#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
Copy link
Member

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
Copy link
Author

ronag commented Apr 18, 2022

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
Copy link
Member

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
Copy link
Author

ronag commented Apr 18, 2022

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

@vweevers
Copy link
Member

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
Copy link
Author

ronag commented Apr 19, 2022

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

@ronag
Copy link
Author

ronag commented Apr 20, 2022

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.

@ronag ronag closed this Apr 26, 2022
@ronag ronag deleted the cleanup branch May 7, 2022 19:56
@jacoscaz
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants