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

add write_buffer_manager setter into options and tests in c bindings, #12007

Conversation

benoitmeriaux
Copy link
Contributor

@benoitmeriaux benoitmeriaux commented Oct 24, 2023

following #11710

  • add test on wbm c api
  • add a setter for WBM in DBOptions

@benoitmeriaux benoitmeriaux marked this pull request as draft October 24, 2023 17:11
@benoitmeriaux benoitmeriaux force-pushed the benoit.meriaux/add-write-buffer-manager-c-bindings branch from 51f5a86 to 46f47dd Compare October 24, 2023 17:56
@benoitmeriaux benoitmeriaux marked this pull request as ready for review October 24, 2023 18:44
@benoitmeriaux benoitmeriaux force-pushed the benoit.meriaux/add-write-buffer-manager-c-bindings branch 6 times, most recently from 6f3d2df to 8dfc528 Compare October 26, 2023 09:11
@benoitmeriaux
Copy link
Contributor Author

@ajkr could you take a look ?

@ajkr
Copy link
Contributor

ajkr commented Nov 9, 2023

Since there are two (also #11710), I'll take the first one - also it allows constructing WBM without a cache object, which is convenient. But 11710 is missing test cases, so if you want to adapt this PR to add the test case, and any APIs that are missing from 11710, I would be happy to merge this too

@benoitmeriaux
Copy link
Contributor Author

I cannot push on the other fork branch, nor fork again since I already have this fork,
so I guess I have to wait for the other PR to be merged, then adjust my PR to add the test and the wbm setter in the option
unless you know a another option @ajkr ?

@ajkr
Copy link
Contributor

ajkr commented Nov 16, 2023

I cannot push on the other fork branch, nor fork again since I already have this fork, so I guess I have to wait for the other PR to be merged, then adjust my PR to add the test and the wbm setter in the option unless you know a another option @ajkr ?

Yes, I think waiting for the other PR to be merged will be the simplest way. Let me try to expedite merging that one. Thanks for your willingness to contribute the test.

@benoitmeriaux benoitmeriaux force-pushed the benoit.meriaux/add-write-buffer-manager-c-bindings branch from 82ed6ee to bc28771 Compare November 17, 2023 10:20
@benoitmeriaux benoitmeriaux changed the title add write_buffer_manager and its setter into options in c bindings, add write_buffer_manager setter into options and tests in c bindings, Nov 17, 2023
@benoitmeriaux benoitmeriaux force-pushed the benoit.meriaux/add-write-buffer-manager-c-bindings branch from bc28771 to 0773b3c Compare November 17, 2023 10:26
@benoitmeriaux
Copy link
Contributor Author

benoitmeriaux commented Nov 17, 2023

I've updated the PR :)

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.

Thanks!

@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.

@facebook-github-bot
Copy link
Contributor

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

@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.

@facebook-github-bot
Copy link
Contributor

@ajkr merged this pull request in 7780e98.

@benoitmeriaux
Copy link
Contributor Author

@ajkr do you know when the next release is planned ?

@ajkr
Copy link
Contributor

ajkr commented Nov 20, 2023

This will be in version 8.9, which should be tagged some time in December.

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.

None yet

3 participants