You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "ninsmiracle (via GitHub)" <gi...@apache.org> on 2024/02/18 12:20:13 UTC
[PR] Support no idempotent dupFeature: support force no idempotent wirte when doing duplication [incubator-pegasus]
ninsmiracle opened a new pull request, #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908
### What problem does this PR solve? <!--add issue link with summary if exists-->
#1907
### What is changed and how does it work?
1. Change the judgment logic in the writing process, and configure it to allow non-idempotent operations in the master cluster.
2. Modify the logic of the specified mutation committer in prepare_list
3. Add configuration items and add them to the replica side to modify them through the server-config of admin-cli.
4. Add the parsing logic after the backup cluster receives the packaged RPC of duplication.
5. Add the logic for the master cluster to obtain relevant non-idempotent RPC from the plog
6. Add the metric index of dup non-idempotent write QPS
##### Tests <!-- At least one of them must be included. -->
- Unit test
- Manual test (add detailed scripts or steps below)
when user want to force dup non-idempotent mutations. Type following command in admin-cli:
```
server-config replica force_send_no_idempotent_when_duplication set true
```
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "ninsmiracle (via GitHub)" <gi...@apache.org>.
ninsmiracle commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1524296202
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -189,6 +211,9 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
// retry this rpc
_inflights[hash].push_front(rpc);
_env.schedule([hash, cb, this]() { send(hash, cb); }, 1_s);
Review Comment:
Yes,it will. So I record specific `raw_key` into log.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1522593316
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -59,6 +60,11 @@ METRIC_DEFINE_counter(replica,
dsn::metric_unit::kRequests,
"The number of failed DUPLICATE requests sent from client");
+METRIC_DEFINE_counter(replica,
+ dup_retry_non_idempotent_duplicate_request,
+ dsn::metric_unit::kRequests,
+ "The qps of Non-idempotent write when doing DUPLICATE which is Retried");
Review Comment:
Maybe ?
```suggestion
"The number of retried non-idempotent DUPLICATE requests sent from client");
```
##########
src/common/duplication_common.cpp:
##########
@@ -37,6 +37,13 @@ DSN_DEFINE_uint32(replication,
"send mutation log batch bytes size per rpc");
DSN_TAG_VARIABLE(duplicate_log_batch_bytes, FT_MUTABLE);
+DSN_DEFINE_bool(replication,
+ force_send_non_idempotent_when_duplication,
Review Comment:
To keep the coding style and convenient distinguish which component the config belongs to, it would be better to rename config names in duplication as `duplication_xxx`, metric names as `dup_xxx`.
For example, force_send_non_idempotent_when_duplication -> duplication_unsafe_allow_non_idempotent, the "unsafe" stands by issue may caused the retry mechanism.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1522480693
##########
src/common/duplication_common.h:
##########
@@ -31,6 +31,7 @@
#include "utils/fmt_utils.h"
DSN_DECLARE_uint32(duplicate_log_batch_bytes);
+DSN_DECLARE_bool(force_send_no_idempotent_when_duplication);
Review Comment:
The DSN_DECLARE_xxx macros are recommended to move to cpp files where actually use it, don't make the header files to be too large.
The same to `DSN_DECLARE_uint32(duplicate_log_batch_bytes); `
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "ninsmiracle (via GitHub)" <gi...@apache.org>.
ninsmiracle commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1510638806
##########
src/common/duplication_common.h:
##########
@@ -31,6 +31,7 @@
#include "utils/fmt_utils.h"
DSN_DECLARE_uint32(duplicate_log_batch_bytes);
+DSN_DECLARE_bool(force_send_no_idempotent_when_duplication);
Review Comment:
I think it's necessary . Cause once I remove this line , compilation can not be passed.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "ninsmiracle (via GitHub)" <gi...@apache.org>.
ninsmiracle commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1510674594
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -205,6 +230,75 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
}
}
+void pegasus_mutation_duplicator::type_force_send_no_idempotent_if_need(duplicate_rpc &rpc)
+{
+
+ // there maybe more than one mutation in one dup rpc
+ if (FLAGS_force_send_no_idempotent_when_duplication) {
+ for (auto entry : rpc.request().entries) {
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_SET ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_MUTATE) {
+
Review Comment:
`entry.task_code` is not a const variable. And I think using task_code.to_string is not more efficient than `if` structure.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "ninsmiracle (via GitHub)" <gi...@apache.org>.
ninsmiracle commented on PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#issuecomment-1965698883
Now , when user config `force_send_no_idempotent_when_duplication` to true . We can dup non-idempotent write to backup cluster , and we can verify the data consistency by check the number rows of pegasus app.
app in master-cluster
app in backup-cluster
And because the retries for non-idempotent writes will make influence of the consistency between two clusters. So we should show the metric like following picture:
show in backup cluster:
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "ninsmiracle (via GitHub)" <gi...@apache.org>.
ninsmiracle commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1523330584
##########
src/common/duplication_common.cpp:
##########
@@ -37,6 +37,13 @@ DSN_DEFINE_uint32(replication,
"send mutation log batch bytes size per rpc");
DSN_TAG_VARIABLE(duplicate_log_batch_bytes, FT_MUTABLE);
+DSN_DEFINE_bool("replication",
+ force_send_no_idempotent_when_duplication,
+ false,
+ "receive client idempotent write requests and send them to backup cluster when "
Review Comment:
sorry. I corrected 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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1522481328
##########
src/common/duplication_common.cpp:
##########
@@ -37,6 +37,13 @@ DSN_DEFINE_uint32(replication,
"send mutation log batch bytes size per rpc");
DSN_TAG_VARIABLE(duplicate_log_batch_bytes, FT_MUTABLE);
+DSN_DEFINE_bool("replication",
+ force_send_no_idempotent_when_duplication,
+ false,
+ "receive client idempotent write requests and send them to backup cluster when "
Review Comment:
Didn't update the description?
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "ninsmiracle (via GitHub)" <gi...@apache.org>.
ninsmiracle commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1523078751
##########
src/common/duplication_common.cpp:
##########
@@ -37,6 +37,13 @@ DSN_DEFINE_uint32(replication,
"send mutation log batch bytes size per rpc");
DSN_TAG_VARIABLE(duplicate_log_batch_bytes, FT_MUTABLE);
+DSN_DEFINE_bool(replication,
+ force_send_non_idempotent_when_duplication,
Review Comment:
OK
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1522682770
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -205,6 +230,75 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
}
}
+void pegasus_mutation_duplicator::type_force_send_no_idempotent_if_need(duplicate_rpc &rpc)
+{
+
+ // there maybe more than one mutation in one dup rpc
+ if (FLAGS_force_send_no_idempotent_when_duplication) {
+ for (auto entry : rpc.request().entries) {
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_SET ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_MUTATE) {
+
+ METRIC_VAR_INCREMENT(dup_retry_no_idempotent_duplicate_qps);
+
+ dsn::message_ex *write =
+ dsn::from_blob_to_received_msg(entry.task_code, entry.raw_message);
+
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR) {
+ incr_rpc raw_rpc(write);
+ absl::string_view unmarshall_key(raw_rpc.request().key.data(),
Review Comment:
Is there any difference? the operator<</to_string()/to_string_view() member function of blob are using data() and length() to create a string/string_view object too.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1522481328
##########
src/common/duplication_common.cpp:
##########
@@ -37,6 +37,13 @@ DSN_DEFINE_uint32(replication,
"send mutation log batch bytes size per rpc");
DSN_TAG_VARIABLE(duplicate_log_batch_bytes, FT_MUTABLE);
+DSN_DEFINE_bool("replication",
+ force_send_no_idempotent_when_duplication,
+ false,
+ "receive client idempotent write requests and send them to backup cluster when "
Review Comment:
Didn't update this?
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1522481783
##########
src/server/pegasus_write_service.cpp:
##########
@@ -121,6 +121,11 @@ METRIC_DEFINE_counter(replica,
dsn::metric_unit::kRequests,
"The number of DUPLICATE requests");
+METRIC_DEFINE_counter(replica,
+ force_receive_no_idempotent_duplicate_qps,
Review Comment:
Didn't update this?
Similar to `dup_requests` above.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "ninsmiracle (via GitHub)" <gi...@apache.org>.
ninsmiracle commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1510697624
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -205,6 +230,75 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
}
}
+void pegasus_mutation_duplicator::type_force_send_no_idempotent_if_need(duplicate_rpc &rpc)
+{
+
+ // there maybe more than one mutation in one dup rpc
+ if (FLAGS_force_send_no_idempotent_when_duplication) {
+ for (auto entry : rpc.request().entries) {
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_SET ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_MUTATE) {
+
+ METRIC_VAR_INCREMENT(dup_retry_no_idempotent_duplicate_qps);
+
+ dsn::message_ex *write =
+ dsn::from_blob_to_received_msg(entry.task_code, entry.raw_message);
+
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR) {
+ incr_rpc raw_rpc(write);
+ absl::string_view unmarshall_key(raw_rpc.request().key.data(),
Review Comment:
In my opinion , `raw_rpc.request().key` is a blob. And `unmarshall_key` is more friendly for Pegasus operation and maintenance person.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1508543840
##########
src/common/duplication_common.cpp:
##########
@@ -37,6 +37,13 @@ DSN_DEFINE_uint32(replication,
"send mutation log batch bytes size per rpc");
DSN_TAG_VARIABLE(duplicate_log_batch_bytes, FT_MUTABLE);
+DSN_DEFINE_bool("replication",
+ force_send_no_idempotent_when_duplication,
+ false,
+ "receive client idempotent write requests and send them to backup cluster when "
Review Comment:
non-idempotent
##########
src/server/pegasus_write_service.cpp:
##########
@@ -121,6 +121,11 @@ METRIC_DEFINE_counter(replica,
dsn::metric_unit::kRequests,
"The number of DUPLICATE requests");
+METRIC_DEFINE_counter(replica,
+ force_receive_no_idempotent_duplicate_qps,
Review Comment:
It's just a counter, it needs a duration to calculate the qps, so don't add qps in the name. You can naming it like "xxx_requests"
##########
src/common/duplication_common.h:
##########
@@ -31,6 +31,7 @@
#include "utils/fmt_utils.h"
DSN_DECLARE_uint32(duplicate_log_batch_bytes);
+DSN_DECLARE_bool(force_send_no_idempotent_when_duplication);
Review Comment:
Is it necessary to add it here?
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -205,6 +230,75 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
}
}
+void pegasus_mutation_duplicator::type_force_send_no_idempotent_if_need(duplicate_rpc &rpc)
+{
+
+ // there maybe more than one mutation in one dup rpc
+ if (FLAGS_force_send_no_idempotent_when_duplication) {
+ for (auto entry : rpc.request().entries) {
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_SET ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_MUTATE) {
+
Review Comment:
Add these codes in a set, then:
```
if (entry.task_code not in set) {
continue;
}
METRIC_VAR_INCREMENT(dup_retry_no_idempotent_duplicate_qps);
switch (entry.task_code) {
case x:
...
case y:
...
}
```
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -205,6 +230,75 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
}
}
+void pegasus_mutation_duplicator::type_force_send_no_idempotent_if_need(duplicate_rpc &rpc)
+{
+
+ // there maybe more than one mutation in one dup rpc
+ if (FLAGS_force_send_no_idempotent_when_duplication) {
+ for (auto entry : rpc.request().entries) {
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_SET ||
+ entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_MUTATE) {
+
+ METRIC_VAR_INCREMENT(dup_retry_no_idempotent_duplicate_qps);
+
+ dsn::message_ex *write =
+ dsn::from_blob_to_received_msg(entry.task_code, entry.raw_message);
+
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR) {
+ incr_rpc raw_rpc(write);
+ absl::string_view unmarshall_key(raw_rpc.request().key.data(),
Review Comment:
Is it possible to use `raw_rpc.request().key` directly in LOG_DEBUG?
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -205,6 +230,75 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
}
}
+void pegasus_mutation_duplicator::type_force_send_no_idempotent_if_need(duplicate_rpc &rpc)
+{
+
+ // there maybe more than one mutation in one dup rpc
+ if (FLAGS_force_send_no_idempotent_when_duplication) {
Review Comment:
Reducce the indents.
```
if (!FLAGS_force_send_no_idempotent_when_duplication) {
return;
}
for (...) {
...
}
```
##########
src/server/pegasus_write_service.cpp:
##########
@@ -415,6 +421,40 @@ int pegasus_write_service::duplicate(int64_t decree,
}
continue;
}
+
+ if (request.task_code == dsn::apps::RPC_RRDB_RRDB_INCR ||
+ request.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_SET ||
+ request.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_MUTATE) {
+ // receive no idempotent request from master cluster via duplication
+ METRIC_VAR_INCREMENT(force_receive_no_idempotent_duplicate_qps);
+
+ if (request.task_code == dsn::apps::RPC_RRDB_RRDB_INCR) {
+ incr_rpc rpc(write);
+ resp.__set_error(_impl->incr(ctx.decree, rpc.request(), rpc.response()));
Review Comment:
Add some comments to describe what's you aim here.
##########
src/common/duplication_common.cpp:
##########
@@ -37,6 +37,13 @@ DSN_DEFINE_uint32(replication,
"send mutation log batch bytes size per rpc");
DSN_TAG_VARIABLE(duplicate_log_batch_bytes, FT_MUTABLE);
+DSN_DEFINE_bool("replication",
Review Comment:
```suggestion
DSN_DEFINE_bool(replication,
```
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1522481783
##########
src/server/pegasus_write_service.cpp:
##########
@@ -121,6 +121,11 @@ METRIC_DEFINE_counter(replica,
dsn::metric_unit::kRequests,
"The number of DUPLICATE requests");
+METRIC_DEFINE_counter(replica,
+ force_receive_no_idempotent_duplicate_qps,
Review Comment:
Didn't update this?
tips: rename it similar to `dup_requests` above.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1522481783
##########
src/server/pegasus_write_service.cpp:
##########
@@ -121,6 +121,11 @@ METRIC_DEFINE_counter(replica,
dsn::metric_unit::kRequests,
"The number of DUPLICATE requests");
+METRIC_DEFINE_counter(replica,
+ force_receive_no_idempotent_duplicate_qps,
Review Comment:
Didn't update this?
tips: rename it similar to `dup_requests` above.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#discussion_r1524133408
##########
src/common/duplication_common.cpp:
##########
@@ -37,6 +37,15 @@ DSN_DEFINE_uint32(replication,
"send mutation log batch bytes size per rpc");
DSN_TAG_VARIABLE(duplicate_log_batch_bytes, FT_MUTABLE);
+DSN_DEFINE_bool(
+ replication,
+ duplication_unsafe_allow_non_idempotent,
+ false,
+ "Turn on the switch so that the cluster can accept non-idempotent writes and forward these "
+ "writes via duplication "
+ "Note that this switch may cause data inconsistency between clusters. So we say it is unsafe ");
Review Comment:
```suggestion
"writes via duplication. Note that this switch may cause data inconsistency between "
"clusters. So we say it is unsafe.");
```
##########
src/common/duplication_common.h:
##########
@@ -31,6 +31,7 @@
#include "utils/fmt_utils.h"
DSN_DECLARE_uint32(duplicate_log_batch_bytes);
+DSN_DECLARE_bool(duplication_unsafe_allow_non_idempotent);
Review Comment:
Move these declarations to the places where actually use them, don't put them in header files.
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -205,6 +230,58 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
}
}
+void pegasus_mutation_duplicator::type_force_send_non_idempotent_if_need(duplicate_rpc &rpc)
+{
+ if (!FLAGS_duplication_unsafe_allow_non_idempotent) {
+ return;
+ }
+
+ // there maybe more than one mutation in one dup rpc
+ for (auto entry : rpc.request().entries) {
+ // not a non idempotent request
+ if (!_non_idempotent_code.count(entry.task_code)) {
+ continue;
+ }
+
+ METRIC_VAR_INCREMENT(dup_retry_non_idempotent_duplicate_request);
+ dsn::message_ex *write = dsn::from_blob_to_received_msg(entry.task_code, entry.raw_message);
+
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR) {
+ incr_rpc raw_rpc(write);
+
+ LOG_DEBUG("Non-indempotent write RPC_RRDB_RRDB_INCR has been retried when doing "
+ "duplication,"
+ "key is [{}]",
+ raw_rpc.request().key);
+ continue;
+ }
+
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_SET) {
+ check_and_set_rpc raw_rpc(write);
+
+ LOG_DEBUG("Non-indempotent write RPC_RRDB_RRDB_CHECK_AND_SET has been retried "
+ "when doing duplication,"
+ "hash key [{}], check sort key [{}],"
+ "set sort key [{}]",
+ raw_rpc.request().hash_key,
+ raw_rpc.request().check_sort_key,
+ raw_rpc.request().set_sort_key);
+ continue;
+ }
+
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_MUTATE) {
+ check_and_mutate_rpc raw_rpc(write);
+
+ LOG_DEBUG("Non-indempotent write RPC_RRDB_RRDB_CHECK_AND_MUTATE has been "
+ "retried when doing duplication,"
+ "hash key is [{}] , sort key is [{}] .",
Review Comment:
```suggestion
"retried when doing duplication, hash key is '{}', sort key is '{}'.",
```
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -205,6 +230,58 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
}
}
+void pegasus_mutation_duplicator::type_force_send_non_idempotent_if_need(duplicate_rpc &rpc)
+{
+ if (!FLAGS_duplication_unsafe_allow_non_idempotent) {
+ return;
+ }
+
+ // there maybe more than one mutation in one dup rpc
+ for (auto entry : rpc.request().entries) {
+ // not a non idempotent request
+ if (!_non_idempotent_code.count(entry.task_code)) {
+ continue;
+ }
+
+ METRIC_VAR_INCREMENT(dup_retry_non_idempotent_duplicate_request);
+ dsn::message_ex *write = dsn::from_blob_to_received_msg(entry.task_code, entry.raw_message);
+
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR) {
+ incr_rpc raw_rpc(write);
+
+ LOG_DEBUG("Non-indempotent write RPC_RRDB_RRDB_INCR has been retried when doing "
+ "duplication,"
+ "key is [{}]",
+ raw_rpc.request().key);
+ continue;
+ }
+
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_SET) {
+ check_and_set_rpc raw_rpc(write);
+
+ LOG_DEBUG("Non-indempotent write RPC_RRDB_RRDB_CHECK_AND_SET has been retried "
+ "when doing duplication,"
+ "hash key [{}], check sort key [{}],"
+ "set sort key [{}]",
Review Comment:
```suggestion
"when doing duplication, hash key '{}', check sort key '{}', set sort "
"key '{}'",
```
##########
src/server/pegasus_mutation_duplicator.h:
##########
@@ -89,8 +94,14 @@ class pegasus_mutation_duplicator : public dsn::replication::mutation_duplicator
size_t _total_shipped_size{0};
+ const std::set<dsn::task_code> _non_idempotent_code = {
Review Comment:
Try to reuse it in some other places, e.g. src/server/pegasus_write_service.cpp
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -205,6 +230,58 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
}
}
+void pegasus_mutation_duplicator::type_force_send_non_idempotent_if_need(duplicate_rpc &rpc)
+{
+ if (!FLAGS_duplication_unsafe_allow_non_idempotent) {
+ return;
+ }
+
+ // there maybe more than one mutation in one dup rpc
+ for (auto entry : rpc.request().entries) {
+ // not a non idempotent request
+ if (!_non_idempotent_code.count(entry.task_code)) {
+ continue;
+ }
+
+ METRIC_VAR_INCREMENT(dup_retry_non_idempotent_duplicate_request);
+ dsn::message_ex *write = dsn::from_blob_to_received_msg(entry.task_code, entry.raw_message);
+
+ if (entry.task_code == dsn::apps::RPC_RRDB_RRDB_INCR) {
+ incr_rpc raw_rpc(write);
+
+ LOG_DEBUG("Non-indempotent write RPC_RRDB_RRDB_INCR has been retried when doing "
+ "duplication,"
+ "key is [{}]",
Review Comment:
```suggestion
"duplication, key is '{}'",
```
##########
src/server/pegasus_mutation_duplicator.h:
##########
@@ -89,8 +94,14 @@ class pegasus_mutation_duplicator : public dsn::replication::mutation_duplicator
size_t _total_shipped_size{0};
+ const std::set<dsn::task_code> _non_idempotent_code = {
Review Comment:
```suggestion
const std::set<dsn::task_code> _non_idempotent_codes = {
```
##########
src/server/pegasus_write_service.cpp:
##########
@@ -415,6 +421,44 @@ int pegasus_write_service::duplicate(int64_t decree,
}
continue;
}
+
+ // Parse non-idempotent writes via duplication
+ if (request.task_code == dsn::apps::RPC_RRDB_RRDB_INCR ||
+ request.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_SET ||
+ request.task_code == dsn::apps::RPC_RRDB_RRDB_CHECK_AND_MUTATE) {
+ // receive non-idempotent request from master cluster via duplication when
+ // FLAG_duplication_unsafe_allow_non_idempotent set as true.
+ // This metric greater than zero means that there is already the possibility of
+ // inconsistency between clusters
Review Comment:
Move these comments to the dup_unsafe_received_non_idempotent_duplicate_request metric description, then users know what does it mean from http API, rather than finding out the comments in code.
##########
src/server/pegasus_mutation_duplicator.cpp:
##########
@@ -189,6 +211,9 @@ void pegasus_mutation_duplicator::on_duplicate_reply(uint64_t hash,
// retry this rpc
_inflights[hash].push_front(rpc);
_env.schedule([hash, cb, this]() { send(hash, cb); }, 1_s);
Review Comment:
Will the non-idempotent request be retried twice here?
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "ninsmiracle (via GitHub)" <gi...@apache.org>.
ninsmiracle commented on PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#issuecomment-1955789587
This PR's base function is in line with expectations , however I was requested by business department to add more matrix for surveillance system about this feature. I will update this PR in recently days.
--
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: support to force send non-idempotent write when doing duplication [incubator-pegasus]
Posted by "ruojieranyishen (via GitHub)" <gi...@apache.org>.
ruojieranyishen commented on PR #1908:
URL: https://github.com/apache/incubator-pegasus/pull/1908#issuecomment-4087493840
new pr is https://github.com/apache/incubator-pegasus/pull/2380
--
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