-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
jni: expose memtable_whole_key_filtering option #9394
Conversation
37d8c07
to
7cfa31a
Compare
7cfa31a
to
fb8d99d
Compare
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.
Please could you change the documentation comment to say "Return boolean indicating whether whole key bloom filter is enabled in memtable". There's not really a mode as such, and it's best when adding to the JNI to directly echo the C++ interface documentation, for clarity.
* | ||
* Default: false (disable) | ||
* | ||
* @param memtableWholeKeyFiltering the memtable whole key filtering mode |
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 think it's better to put "true iff whole key bloom filter is enabled in memtable" - it's not really a mode.
T setMemtableWholeKeyFiltering(boolean memtableWholeKeyFiltering); | ||
|
||
/** | ||
* Returns whether a memtable whole key filtering mode will be used |
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.
Same thing about comments "Returns whether whole key bloom filter is enabled in memtable"
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.
Nice little change. Just a couple of changes I've suggested to the text of method documentation, to keep them as consistent as possible with the C++ documentation.
@alanpaxton Thanks for your review. The documentation issue has been resolved 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.
LGTM
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
refer to: https://github.com/facebook/rocksdb/wiki/Prefix-Seek#configure-prefix-bloom-filter