-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Java API for Merge Operator v2 #12122
base: main
Are you sure you want to change the base?
Conversation
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.
A few small things to tidy up. Looks good apart from those.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I check existing code and we don't throw exceptions when we can't create global references.
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.
Really ? That seems wrong, and may be an omission. If NewGlobalRef() is returning null, surely the right thing to do is to throw an exception, unless we somehow cope with j_merge_class as null ? So let us do it correctly here for now.
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.
@alanpaxton Fixed.
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.
Thanks. I'm happy.
80ea516
to
b6dc8f8
Compare
b6dc8f8
to
32d42b3
Compare
82606aa
to
2b4c318
Compare
2b4c318
to
cc0bb1a
Compare
cc0bb1a
to
96642ee
Compare
96642ee
to
02fcc10
Compare
02fcc10
to
41a2331
Compare
41a2331
to
c9194d2
Compare
3aa81ea
to
ff5b251
Compare
@rhubner LGTM now in respect of changes I requested on previous review. |
ff5b251
to
5e0e913
Compare
5e0e913
to
9947574
Compare
9947574
to
971f57a
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.
971f57a
to
a6c5c60
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.