RocksJava API - fix Transaction.multiGet() size limit, remove bogus EnsureLocalCapacity() calls #10674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alanpaxton
force-pushed
the
fb-9006-transaction-multiget-large-keys
branch
from
897a791
to
e522825
Compare
alanpaxton
force-pushed
the
fb-9006-transaction-multiget-large-keys
branch
from
e522825
to
e7fe8af
Compare
alanpaxton
changed the title
alanpaxton
force-pushed
the
fb-9006-transaction-multiget-large-keys
branch
from
b0adb4a
to
675ec7c
Compare
alanpaxton
changed the title
@see facebook#9006 Instances of RocksJava API JNI code appear to have misunderstood the reason for JNIEnv->EnsureLocalCapacity() and are carrying out bogus checks which happen to fail with some larger parameter values (many column familes in a single call, very long key names or values). Remove these checks and add some regression tests for the previous failures. Add/update tests to ensure that methods with EnsureLocalCapacity() removed still work; testing was perhaps a bit thin in places.
In conceptually similar (though not identical, APIs are a bit mismatched) way to standard multiGet, keys are now copied into jbyte[] so that local object references need only be held one at a time inside a loop.
Tests are systematic, write values to 10% of keys, handle duplicate keys (byte[] equality needs wrapped for proper value equals()).
alanpaxton
force-pushed
the
fb-9006-transaction-multiget-large-keys
branch
from
675ec7c
to
caf62e9
Compare
|
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@alanpaxton has updated the pull request. You must reimport the pull request before landing. |
|
@jay-zhuang has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves @see #9006
Fixes 2 related issues with JNI local references in the RocksJava API.
Some instances of RocksJava API JNI code appear to have misunderstood the reason for
JNIEnv->EnsureLocalCapacity()and are carrying out bogus checks which happen to fail with some larger parameter values (many column families in a single call, very long key names or values). Remove these checks and add some regression tests for the previous failures.The helper for Transaction multiGet operations (
multiGet(),multiGetForUpdate(),...) is limited in the number of keys it canget()for because it requires a corresponding number of live local references. Refactor the helper slightly, copying out the key contents within a loop so that the references don't have to exist at the same time.