You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "empiredan (via GitHub)" <gi...@apache.org> on 2025/02/10 10:18:34 UTC
[PR] feat(make_idempotent): support making `incr` request idempotent in `pegasus_write_service` [incubator-pegasus]
empiredan opened a new pull request, #2192:
URL: https://github.com/apache/incubator-pegasus/pull/2192
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(make_idempotent): support making `incr` request idempotent in `pegasus_write_service` [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #2192:
URL: https://github.com/apache/incubator-pegasus/pull/2192#discussion_r1957692030
##########
src/server/pegasus_write_service.h:
##########
@@ -202,6 +216,12 @@ class pegasus_write_service : dsn::replication::replica_base
uint64_t _batch_start_time;
+ // Only used for primary replica to calculate the duration that an incr request from
+ // 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;
Review Comment:
I see, thanks for the clarifying.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(make_idempotent): support making `incr` request idempotent in `pegasus_write_service` [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #2192:
URL: https://github.com/apache/incubator-pegasus/pull/2192#discussion_r1956344401
##########
src/server/pegasus_write_service.cpp:
##########
@@ -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()
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(make_idempotent): support making `incr` request idempotent in `pegasus_write_service` [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #2192:
URL: https://github.com/apache/incubator-pegasus/pull/2192#discussion_r1954705187
##########
src/server/pegasus_write_service.cpp:
##########
@@ -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()
Review Comment:
The function name is not accurate, not only 'clean up', but also update the metrics.
##########
src/server/pegasus_write_service.h:
##########
@@ -202,6 +216,12 @@ class pegasus_write_service : dsn::replication::replica_base
uint64_t _batch_start_time;
+ // Only used for primary replica to calculate the duration that an incr request from
+ // 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;
Review Comment:
This is a member variable, which will be shared by all requests. But it's a request related latency, isn't it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(make_idempotent): support making `incr` request idempotent in `pegasus_write_service` [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan commented on code in PR #2192:
URL: https://github.com/apache/incubator-pegasus/pull/2192#discussion_r1956339192
##########
src/server/pegasus_write_service.h:
##########
@@ -202,6 +216,12 @@ class pegasus_write_service : dsn::replication::replica_base
uint64_t _batch_start_time;
+ // Only used for primary replica to calculate the duration that an incr request from
+ // 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;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org
Re: [PR] feat(make_idempotent): support making `incr` request idempotent in `pegasus_write_service` [incubator-pegasus]
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan merged PR #2192:
URL: https://github.com/apache/incubator-pegasus/pull/2192
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org