Skip to content
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] Fix multiget throwing NPE for num of keys > 70k #9012

Closed
wants to merge 4 commits into from

Conversation

alanpaxton
Copy link
Contributor

closes #8039

Unnecessary use of multiple local JNI references at the same time, 1 per key, was limiting the size of the key array. The local references don't need to be held simultaneously, so if we rearrange the code we can make it work for bigger key arrays.

Incidentally, make errors throw helpful exception messages rather than returning a null pointer.

Alan Paxton added 3 commits October 11, 2021 10:09
Unnecessary use of multiple local JNI references at the same time, 1 per key, was limiting the size of the key array. The local references don't really need to be held simultaneously, so if we shuffle things around a bit we can make it work for bigger key arrays. Also, make errors throw helpful exception messages rather than returning a null pointer.
@adamretter
Copy link
Collaborator

Thanks @alanpaxton this LGTM. Could @jay-zhuang or @mrambacher merge this one please?

@adamretter adamretter self-requested a review October 12, 2021 17:51
@facebook-github-bot
Copy link
Contributor

@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ajkr
Copy link
Contributor

ajkr commented Oct 13, 2021

Is it also expected to fix #9006 ?

@alanpaxton
Copy link
Contributor Author

No @ajkr I was unaware of #9006 and the parallel transactional multiget code. That has its own helper functions with the same flaws. Perhaps check with @adamretter and agree whether I should expand the work here to catch that, or accept this as-is and leave the other ticket open ?

@facebook-github-bot
Copy link
Contributor

@alanpaxton has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@jay-zhuang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jay-zhuang jay-zhuang changed the title Fb 8039 multiget npe 70k [JAVA] Fix multiget throwing NPE for num of keys > 70k Oct 14, 2021
@facebook-github-bot
Copy link
Contributor

@jay-zhuang merged this pull request in f5526af.

@asad-awadia
Copy link

@alanpaxton how is performance for you when doing a large multi get? [concurrently]

yoori pushed a commit to yoori/rocksdb that referenced this pull request Nov 26, 2023
Summary:
closes facebook/rocksdb#8039

Unnecessary use of multiple local JNI references at the same time, 1 per key, was limiting the size of the key array. The local references don't need to be held simultaneously, so if we rearrange the code we can make it work for bigger key arrays.

Incidentally, make errors throw helpful exception messages rather than returning a null pointer.

Pull Request resolved: facebook/rocksdb#9012

Reviewed By: mrambacher

Differential Revision: D31580862

Pulled By: jay-zhuang

fbshipit-source-id: ce05831d52ede332e1b20e74d2dc621d219b9616
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Java MultiGet NPE with key list 70_000 , but ok with 60_000
6 participants