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

Add db.getMany(keys) across the board #101

Closed
15 tasks done
vweevers opened this issue Sep 25, 2021 · 8 comments
Closed
15 tasks done

Add db.getMany(keys) across the board #101

vweevers opened this issue Sep 25, 2021 · 8 comments
Labels
enhancement New feature or request

Comments

@vweevers
Copy link
Member

vweevers commented Sep 25, 2021

Background: Level/abstract-leveldown#380 and Level/levelup#491.

@vweevers vweevers added the enhancement New feature or request label Sep 25, 2021
vweevers added a commit to Level/leveldown that referenced this issue Sep 25, 2021
vweevers added a commit to Level/memdown that referenced this issue Sep 25, 2021
vweevers added a commit to Level/deferred-leveldown that referenced this issue Sep 25, 2021
vweevers added a commit to Level/encoding-down that referenced this issue Sep 25, 2021
vweevers added a commit to Level/levelup that referenced this issue Sep 25, 2021
vweevers added a commit to Level/levelup that referenced this issue Sep 25, 2021
vweevers added a commit to Level/level-js that referenced this issue Sep 25, 2021
vweevers added a commit to Level/abstract-leveldown that referenced this issue Sep 28, 2021
vweevers added a commit to Level/leveldown that referenced this issue Sep 28, 2021
vweevers added a commit to Level/memdown that referenced this issue Sep 28, 2021
vweevers added a commit to Level/leveldown that referenced this issue Sep 28, 2021
vweevers added a commit to Level/level-js that referenced this issue Sep 28, 2021
vweevers added a commit to Level/memdown that referenced this issue Sep 28, 2021
vweevers added a commit to Level/deferred-leveldown that referenced this issue Sep 28, 2021
vweevers added a commit to Level/level-js that referenced this issue Sep 28, 2021
vweevers added a commit to Level/encoding-down that referenced this issue Sep 29, 2021
vweevers added a commit to Level/deferred-leveldown that referenced this issue Sep 30, 2021
vweevers added a commit to Level/encoding-down that referenced this issue Sep 30, 2021
Ref Level/community#101

Also adds the abstract-leveldown test suite.

This works thanks to the `encodings` option that was added to the
test suite (originally for `levelup` compatibility testing) and
changes the expected outputs (from buffers by default to strings by
default).
@MeirionHughes
Copy link
Member

MeirionHughes commented Oct 1, 2021

I'll try with the rockdb port to help out if no one was already doing it. I also need to check the typings on DT as they're broken after the 'default' export removals. So I'll start adding getMany at same time.

@vweevers
Copy link
Member Author

vweevers commented Oct 1, 2021

Go for it, thanks! Note that on rocksdb we must first port Level/leveldown#783, Level/leveldown#784, Level/leveldown#785 (in that order), and then Level/leveldown#787.

@gogoout
Copy link

gogoout commented Oct 1, 2021

I'll try with the rockdb port to help out if no one was already doing it. I also need to check the typings on DT as they're broken after the 'default' export removals. So I'll start adding getMany at same time.

Just asking, do you plan to use the rocksdb's native getMany interface to implementing this?

@vweevers
Copy link
Member Author

vweevers commented Oct 1, 2021

I would prefer that we don't, because it will increase the code diff between leveldown and rocksdb, making cherry-picking commits from one to the other more work. In addition, the leveldown code is written in anticipation of additional features like iterator.all() which will reuse common functions.

@MeirionHughes
Copy link
Member

MeirionHughes commented Oct 1, 2021

Just asking, do you plan to use the rocksdb's native getMany interface to implementing this?

That is what I was looking to do...

I would prefer that we don't, because it will increase the code diff between leveldown and rocksdb

... but I guess not. :D

I don't think I know enough to do it tbh: the low-hanging method I was targetting to call:

  virtual std::vector<Status> MultiGet(const ReadOptions& options,
                                       const std::vector<Slice>& keys,
                                       std::vector<std::string>* values) {

has the comment:

Consistent Get of many keys across column families without the need
for an explicit snapshot. NOTE: the implementation of this MultiGet API
does not have the performance benefits of the void-returning MultiGet
functions.

So:

  • a) the Worker is doing options_.snapshot = database->NewSnapshot(); and I'm not sure if (as per comment) whether this will be used. and
  • b) that bar basic reduction in cross-domain calls, that there would actually be performance benefit of changing it to rocksdb's native MultiGet unless it also handles passing through column_family handles.

Probably makes sense to just do all cherry picking for now.

vweevers added a commit to Level/levelup that referenced this issue Oct 1, 2021
vweevers added a commit to Level/multileveldown that referenced this issue Oct 1, 2021
@gogoout
Copy link

gogoout commented Oct 6, 2021

@vweevers @MeirionHughes Just confirming anyone of you are going to implementing this for rocksdb? Just eager to try the new interface :)

@sebastianst
Copy link

Types are still missing getMany. Opened an issue in the levelup repo.

@Bonaventure00916

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

No branches or pull requests

5 participants