[RocksJava] Allow iterators to be refreshed with new data #3465
Conversation
| */ | ||
| void Java_org_rocksdb_WBWIRocksIterator_refresh0( | ||
| JNIEnv* env, jobject jobj, jlong handle) { | ||
| auto* it = reinterpret_cast<rocksdb::Iterator*>(handle); |
There was a problem hiding this comment.
Note the cast to rocksdb::Iterator here instead of rocksdb::WBWIIterator, so that we can use the default implementation and return Status::NotSupported.
There was a problem hiding this comment.
I don't see WBWIIterator to be a child class of Iterator. I don't know how this can be casted.
There was a problem hiding this comment.
I guess you are right. This seems like a blunder on my part. Thanks for catching this.
RocksIterator and WBWIRocksIterator inherit from AbstractRocksIterator/RocksInterface on the java side, but they don't have such an inheritance chain on the C++ side, which contributed to the confusion at my end.
I need to re-think the strategy for WBWIRockIterator ... may be something like just throwing a RocksDBException from Java layer without even going to the C++ layer.
|
Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired. Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status. |
|
Otherwise LGTM |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| return; | ||
| } | ||
|
|
||
| rocksdb::RocksDBExceptionJni::ThrowNew(env, s); |
There was a problem hiding this comment.
@sagar0 if you delete line 549 to line 554, and only leave 556 with a NotSupported message, I can merge it.
We need you advice on how we should do with it.
|
Any news on this? I was going to open the same exact PR but noticed this - I'd love to get access to |
Summary: This PR exposes the `Iterator::Refresh` method to the Java API by adding it on the `RocksIteratorInterface` interface. There are three concrete implementations: `RocksIterator`, `SstFileReaderIterator`, and `WBWIRocksIterator`. For the first two cases, the JNI side simply delegates to the underlying `Iterator::Refresh` method; in the last case, as it doesn't share an ancestor, and per the discussion in #3465, a `Status::NotSupported` exception is thrown. As the last PR had no activity in a while, I'm opening a new one - I'm completely fine with merging the previous PR if it gets completed before this is reviewed. Let me know if there's anything missing or anything else I can do 👍 Pull Request resolved: #6573 Reviewed By: cheng-chang Differential Revision: D20604666 Pulled By: pdillinger fbshipit-source-id: 4de17df1180c3b87b76cfdd77b674b81fc0563f7
Ported Iterator::Refresh functionality to RocksJava. This was originally introduced in the C++ API by @siying in #2621.
This should reduce GC pressure particularly in use-cases that frequently create and destroy large number of iterators just to get to the latest snapshot of data.
The API can be invoked by calling
RocksIterator::refresh().WBWIRocksIterator::refresh()is not implemented and would throw a not-supported exception for now.Test Plan:
Added Unit tests.