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 public API WriteWithCallback to support custom callbacks #12603

Closed
wants to merge 4 commits into from

Conversation

jowlyzhang
Copy link
Contributor

@jowlyzhang jowlyzhang commented May 1, 2024

This PR adds a DB::WriteWithCallback API that does the same things as DB::Write while takes an argument UserWriteCallback to execute custom callback functions during the write.

We currently support two types of callback functions: OnWriteEnqueued and OnWalWriteFinish. The former is invoked after the write is enqueued, and the later is invoked after WAL write finishes when applicable.

These callback functions are intended for users to use to improve synchronization between concurrent writes, their execution is on the write's critical path so it will impact the write's latency if not used properly. The documentation for the callback interface mentioned this and suggest user to keep these callback functions' implementation minimum.

Although transaction interfaces' writes doesn't yet allow user to specify such a user write callback argument, the DBImpl::Write* type of APIs do not differentiate between regular DB writes or writes coming from the transaction layer when it comes to supporting this UserWriteCallback. These callbacks works for all the write modes including: default write mode, Options.two_write_queues, Options.unordered_write, Options.enable_pipelined_write

Test plan:
Added unit test in ./write_callback_test

@jowlyzhang jowlyzhang marked this pull request as draft May 1, 2024 23:53
@jowlyzhang jowlyzhang requested review from anand1976 and ajkr May 2, 2024 00:14
Copy link
Contributor

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

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

How does the user specify the callback? In WriteOptions perhaps?

@jowlyzhang
Copy link
Contributor Author

How does the user specify the callback? In WriteOptions perhaps?

Yeah, that's a good question. WriteOptions would work, another idea is maybe we can make WriteWithCallback a public API too?

@anand1976
Copy link
Contributor

How does the user specify the callback? In WriteOptions perhaps?

Yeah, that's a good question. WriteOptions would work, another idea is maybe we can make WriteWithCallback a public API too?

Yeah, WriteWithCallback should be fine too. We should have an overload that doesn't take WriteCallback as that seems specific to transaction DB, and maybe name PostWalWriteCallback to something more generic.

@jowlyzhang jowlyzhang requested a review from anand1976 May 2, 2024 21:59
Copy link
Contributor

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the PR and quick turnaround

@jowlyzhang
Copy link
Contributor Author

LGTM! Thanks for the PR and quick turnaround

Thank you for the quick review!

@jowlyzhang jowlyzhang force-pushed the cb_after_wal_writes branch from 9fce675 to 477b6b5 Compare May 24, 2024 22:52
@jowlyzhang jowlyzhang changed the title Prototype invoking a post wal write callback in pipelined write mode Add public API WriteWithCallback to support custom callbacks May 24, 2024
@jowlyzhang jowlyzhang marked this pull request as ready for review May 24, 2024 23:35
@jowlyzhang jowlyzhang requested a review from anand1976 May 24, 2024 23:36
Copy link
Contributor

@anand1976 anand1976 left a comment

Choose a reason for hiding this comment

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

LGTM. A couple of minor nits.

include/rocksdb/user_write_callback.h Outdated Show resolved Hide resolved
utilities/transactions/write_prepared_txn.cc Outdated Show resolved Hide resolved
@jowlyzhang jowlyzhang force-pushed the cb_after_wal_writes branch from 477b6b5 to b359e4f Compare June 1, 2024 00:14
@jowlyzhang jowlyzhang force-pushed the cb_after_wal_writes branch from b359e4f to afa1f71 Compare June 1, 2024 00:17
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@jowlyzhang merged this pull request in fc59d8f.

@jowlyzhang jowlyzhang deleted the cb_after_wal_writes branch June 3, 2024 16:49
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