Java API for Merge Operator v2 #12122
Conversation
| db.merge(KEY, "value2".getBytes()); | ||
| db.merge(KEY, "value3".getBytes()); | ||
| byte[] valueFromDb = db.get(KEY); | ||
| assertThat(valueFromDb).containsExactly("value2".getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Can you throw a RuntimeException instead of expecting a value in the code that doesn't get reached ? it's confusing otherwise.
| } | ||
|
|
||
| /** | ||
| * All parameters of this method are valid only during the call. If you want to keep this, you |
There was a problem hiding this comment.
"This is the callback method which supplies the values to be merged for the provided key. The parameters are only valid until the method returns, and must be deep copied if you wish to retain them.
The method should return the result of merging the supplied values with the implemented merge algorithm."
Please also comment the individual parameters.
| db.merge(KEY, "value2".getBytes()); | ||
| db.merge(KEY, "value3".getBytes()); | ||
| byte[] valueFromDb = db.get(KEY); | ||
| assertThat(valueFromDb).containsExactly("value2".getBytes(StandardCharsets.UTF_8)); |
There was a problem hiding this comment.
Can you throw a RuntimeException instead of expecting a value in the code that doesn't get reached ? it's confusing otherwise.
| } | ||
|
|
||
| @Override | ||
| public long nativeHandler() { |
There was a problem hiding this comment.
There is already a getNativeHandle() method on RocksCallbackObject and RocksObject. Shouldn't this be getNativeHandle() too ? You may need to fix inheritance a bit ?
| package org.rocksdb; | ||
|
|
||
| public interface MergeOperator { | ||
| long nativeHandler(); |
There was a problem hiding this comment.
See above, getNativeHandle()
| } | ||
|
|
||
| @Override | ||
| final public long nativeHandler() { |
| char* operator_name = reinterpret_cast<char*>(_operator_name); | ||
|
|
||
| auto* jni_merge_operator = | ||
| new std::shared_ptr<ROCKSDB_NAMESPACE::MergeOperator>( |
There was a problem hiding this comment.
I think this can be std::make_shared(...), which is preferred..
There was a problem hiding this comment.
I don't think so. We need to create a new std:pointer which won't be discarded after this method finishes.
| if (j_merge_class == nullptr) { | ||
| return; // Exception | ||
| } | ||
| j_merge_class = static_cast<jclass>(env->NewGlobalRef(j_merge_class)); |
There was a problem hiding this comment.
The JNI manual says NewGlobalRef() can return nullptr WITHOUT an exception, e.g. if the system is out of memory. So we should throw a Java exception here. Can you also check the other JNI methods you call, to confirm that they always set an exception on nullptr, and if they don't we should set one in each case.
Then fix the comments to say "XYZException has been thrown", or whatever is appropriate.
There was a problem hiding this comment.
I check existing code and we don't throw exceptions when we can't create global references.
rhubner
force-pushed
the
eb/merge-operator-v2
branch
3 times, most recently
from
80ea516
to
b6dc8f8
Compare
rhubner
force-pushed
the
eb/merge-operator-v2
branch
from
b6dc8f8
to
32d42b3
Compare
rhubner
force-pushed
the
eb/merge-operator-v2
branch
3 times, most recently
from
82606aa
to
2b4c318
Compare
rhubner
force-pushed
the
eb/merge-operator-v2
branch
from
2b4c318
to
cc0bb1a
Compare
rhubner
force-pushed
the
eb/merge-operator-v2
branch
from
cc0bb1a
to
96642ee
Compare
This is the first implementation of the merge operator API, where merge operators can be implemented in Java without any C++ code. It prefers simplicity and community feedback is appreciated.
rhubner
force-pushed
the
eb/merge-operator-v2
branch
from
96642ee
to
02fcc10
Compare
This is the first implementation of the merge operator API, where merge operators can be implemented in Java without any C++ code. It prefers simplicity and community feedback is appreciated.