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

feat(make_idempotent): support making incr request idempotent in pegasus_write_service #2192

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

empiredan
Copy link
Contributor

@empiredan empiredan commented Feb 10, 2025

In #2185, we've implemented two APIs
make_idempotent() and put() in pegasus_write_service::impl to support idempotent
incr operations. As the higher-level abstraction, pegasus_write_service is able to process
idempotent incr requests. It will also implement make_idempotent() and put() by calling
APIs provided by pegasus_write_service::impl internally. Similarly, both APIs are actually also
provided for higher-level abstraction.

@empiredan empiredan marked this pull request as ready for review February 11, 2025 07:13
// the client is translated into an idempotent put request before appended to plog,
// including reading the current value from RocksDB and incrementing it by a given
// amount.
uint64_t _make_incr_idempotent_duration_ns;
Copy link
Member

Choose a reason for hiding this comment

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

This is a member variable, which will be shared by all requests. But it's a request related latency, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variable is defined as per-replica rather than per-request, for the reason that the current design for implementing idempotence is to make sure there is only one atomic request being processed in the write pipeline for each replica. This pipeline consists of the following stages:
(1) read the current value from RocksDB and built the idempotent request based on it;
(2) append the corresponding mutation to plog;
(3) broadcast the prepare requests;
(4) apply the result for atomic operation back to RocksDB ultimately.

For a request, this variable will be set in stage (1) and read in stage (4); since there is only one request in the pipeline, this variable is guaranteed not to be set for another request before stage (4) is finished. Therefore, it is safe to define this variable as per-replica.

@@ -326,7 +375,7 @@ void pegasus_write_service::set_default_ttl(uint32_t ttl) { _impl->set_default_t

void pegasus_write_service::clear_up_batch_states()
Copy link
Member

Choose a reason for hiding this comment

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

The function name is not accurate, not only 'clean up', but also update the metrics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could name this function batch_finish() as opposed to batch_prepare: finish batch write with metrics such as latencies calculated and some states cleared.

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.

2 participants