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

[C] Port the batched version of MultiGet() to RocksDB's C API #9952

Closed
wants to merge 1 commit into from

Conversation

yhchiang-sol
Copy link
Contributor

Summary:
The batched version of MultiGet() is not available in RocksDB's C API.
This PR implements rocksdb_batched_multi_get_cf which is a C wrapper function
that invokes the batched version of MultiGet() which takes one single column family.

Test Plan:
Added a new test case under "columnfamilies" test case in c_test.cc

@yhchiang-sol
Copy link
Contributor Author

The error on Travis CI - Pull Request seems transient and not related to this PR.

The command "sudo -E apt-add-repository -y "ppa:ubuntu-toolchain-r/test"" failed and exited with 1 during .

@yhchiang-sol
Copy link
Contributor Author

Hello, @siying, @ajkr. This is YH :p.

We want to use the batched version of MultiGet in our use case, but this PR is a blocker because the function is not available in the C API.

Do you have time to take a look or happen to know who might be the best person to review?
Thank you!

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi YH :), thanks for the PR. LGTM!

@yhchiang-sol
Copy link
Contributor Author

Thank you, @ajkr. Should I rebase or it's good to merge?

@ajkr
Copy link
Contributor

ajkr commented May 11, 2022

Thank you, @ajkr. Should I rebase or it's good to merge?

Should be good to merge - just waiting for someone to accept the internal diff.

@yhchiang-sol
Copy link
Contributor Author

Should be good to merge - just waiting for someone to accept the internal diff.

Thank you. Let me know if there's anything that I should address.

@yhchiang-sol
Copy link
Contributor Author

Hello, @ajkr. Can I know whether the PR looks good internally?

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.

3 participants