-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
Support merge operators in java #4805
base: main
Are you sure you want to change the base?
Support merge operators in java #4805
Conversation
@gfouquier thanks for contributing this change, its amazing. Would you be able to rebase on top of the latest? Also it would be nice to write some comments explaining the interaction between the various java and c++ code files. |
abstract public String name(); | ||
|
||
@Override | ||
protected long initializeNative(final long... nativeParameterHandles) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this method called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must read that part again, but it's probably from the portal class AssociativeMergeOperatorJni which call this : RocksDBNativeClass::getJClass(env, "org/rocksdb/AbstractAssociativeMergeOperator");
I am currently trying to rebase the PR, but I need some time since I didn't read this code since last year. |
…eters into account
rename NotAssociativeMergeOperator as MergeOperator Cosmetic changes: sort includes, add description, remove commented parts, add missing licenses
eaf3baf
to
5747d69
Compare
5747d69
to
731e3a4
Compare
The PR is now up to date.
I mimic existing behaviour between java and c++ files, so I don't think this should appear as comment in this PR files specifically.
|
void JniCallback::catchAndLog(JNIEnv* env) const { | ||
jboolean hasException = env->ExceptionCheck(); | ||
if (hasException == JNI_TRUE) { | ||
env->ExceptionDescribe(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this just write to StdOut/StdErr?
It would be better to use the Logger
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure how to do this. I saw that ExceptionDescribe was used in other places too. Maybe it should be part of a more global ticket for jni env exception handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this looks good, thank you.
It just needs a small bit of cleanup around error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated version with a lot more checking around jni calls and some minor fixes.
When will this PR merge ? |
It looks like this PR has been untouched for close to a year. Is there any possibility of moving this forward soon? |
Support for merge operator in java.
Add AbstractAssociativeMergeOperator and AbstractMergeOperator which allow to implement merge operators in java. The MergeOperator class in java now inherit from RocksCallBacksObject and existing merge operators have been rewritten accordingly. In c++, AssociativeMergeOperatorJniCallback and MergeOperatorJniCallback inherit from JniCallback.
This PR is based on #3432 but without thread JNI management neither loader management for static variables. Performances are slow since jni thread must be attached/detached at each call.
resolve #2282