Missing MultiGet batched support for JNI Transactions

[vjeko]

Note: Please use Issues only for bug reports. For questions, discussions, feature requests, etc. post to dev group: https://groups.google.com/forum/#!forum/rocksdb or https://www.facebook.com/groups/rocksdb.dev

Expected behavior

Effectively there is no way for a Java client to leverage a batched version of MultiGet API from a transaction:

  // Batched version of MultiGet - see DBImpl::MultiGet(). Sub-classes are
  // expected to override this with an implementation that calls
  // DBImpl::MultiGet()
  virtual void MultiGet(const ReadOptions& options,
                        ColumnFamilyHandle* column_family,
                        const size_t num_keys, const Slice* keys,
                        PinnableSlice* values, Status* statuses,
                        const bool /*sorted_input*/ = false);

Actual behavior

All Java_org_rocksdb_Transaction_multiGet_ JNI APIs end up invoking one of the following MultiGet implementations:

  virtual std::vector<Status> MultiGet(
      const ReadOptions& options,
      const std::vector<ColumnFamilyHandle*>& column_family,
      const std::vector<Slice>& keys, std::vector<std::string>* values) = 0;

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

Steps to reproduce the behavior

None required.

[vjeko]

@jowlyzhang Given that the issue is labeled as up-for-grabs, does this mean you would accept an outside pull request?

[jowlyzhang]

@jowlyzhang Given that the issue is labeled as up-for-grabs, does this mean you would accept an outside pull request?

Yes, we welcome community contributions to RocksDB's java support. Hello @adamretter, just want to double check with you if this is the case.

[adamretter]

just want to double check with you if this is the case.

@jowlyzhang Absolutely. If a PR for RocksJava is sent we will review it and add feedback

@alanpaxton I just wanted to check if perhaps you already had something like this done in an outstanding PR internally?

[alanpaxton]

@adamretter This is falling out of what I'm doing at the moment with WIP in draft PR #11827

[vjeko]

Thanks @adamretter. We'll be forking RocksDB internally and can provide a pull request once ready. There are other issues we have discovered and this seems like the easiest route so far.

[adamretter]

@vjeko It looks like @alanpaxton may already be tackling that... see above...

[alanpaxton]

Hi @vjeko I have been refactoring the support code for multiGet (transactional and non-transactional) and as part of these changes I will try to pick up the use of the batched methods; I have it in the default CF case, but it should also be easy to do for the methods which pass a list of CFs, in the case where this list is a single element. I want to put together a little more jmh testing to validate that we do in fact get the performance gain which we would expect.

[vjeko]

Thanks a lot @alanpaxton! Any ideas on when we can expect your patch in master?

[alanpaxton]

Hi @vjeko the PR (#11827) is still draft because it is waiting on meta to accept another change which will require a slight revisit to this work; but I am endeavouring to clear my RocksDB work-in-progress as best I can :-)