acelyc111 commented on code in PR #2249:
URL:
https://github.com/apache/incubator-pegasus/pull/2249#discussion_r2107554700
##########
src/replica/replica_2pc.cpp:
##########
@@ -255,6 +260,7 @@ bool replica::need_make_idempotent(const task_spec *spec)
const
return false;
}
+ // Only atomic write requests need to be made idempotent.
return !spec->rpc_request_is_write_idempotent;
Review Comment:
In app_info, it is named as `atomic_idempotent`, in task_spec, it is named
as `write_idempotent`, is there some particular consideration?
##########
src/replica/replica_2pc.cpp:
##########
@@ -239,13 +239,18 @@ void replica::on_client_write(dsn::message_ex *request,
bool ignore_throttling)
bool replica::need_reject_non_idempotent(const task_spec *spec) const
{
if (!is_duplication_master()) {
+ // This is not the dup master that needs to duplicate writes to
followers, thus
+ // non-idempotent requests are accepted.
return false;
}
if (_app_info.atomic_idempotent) {
+ // Since the table which this replica belongs to has been configured
to make
+ // all atomic write requests idempotent, certainly they are accepted.
return false;
}
+ // Any atomic write request should be rejected.
Review Comment:
"!spec->rpc_request_is_write_idempotent" and "Any atomic write request" are
equivalent, right? If they are, clarify this in comments. Otherwise, readers
may consider they are subset relationship.
##########
src/server/pegasus_write_service.h:
##########
@@ -217,6 +234,28 @@ class pegasus_write_service :
dsn::replication::replica_base
// Finish batch write with metrics such as latencies calculated and some
states cleared.
void batch_finish();
+ // Used to store the batch size for each type of write into an array, see
comments for
+ // `_batch_sizes` for details.
+ enum class batch_write_type : uint32_t
+ {
+ put = 0,
+ remove,
+ incr,
+ check_and_set,
+ check_and_mutate,
+ count,
Review Comment:
`count` is not a write_type, it indicates the count of write_types? How
about using `COUNT` to clarify it?
##########
src/replica/replica_2pc.cpp:
##########
@@ -265,37 +271,43 @@ bool replica::need_make_idempotent(message_ex *request)
const
}
if (!_app_info.atomic_idempotent) {
+ // The table which this replica belongs to is not configured to make
all atomic
+ // write requests idempotent.
return false;
}
const auto *spec = task_spec::get(request->rpc_code());
- CHECK_NOTNULL(spec, "RPC code {} not found", request->rpc_code());
+ CHECK_NOTNULL_PREFIX_MSG(spec, "RPC code {} not found",
request->rpc_code());
+ // Only atomic write requests need to be made idempotent.
return !spec->rpc_request_is_write_idempotent;
}
int replica::make_idempotent(mutation_ptr &mu)
{
- CHECK_TRUE(!mu->client_requests.empty());
+ CHECK_PREFIX_MSG(!mu->client_requests.empty(),
+ "the mutation should include at least one request");
message_ex *request = mu->client_requests.front();
if (!need_make_idempotent(request)) {
return rocksdb::Status::kOk;
}
- // The original atomic write request must not be batched.
- CHECK_EQ(mu->client_requests.size(), 1);
+ CHECK_EQ_PREFIX_MSG(
+ mu->client_requests.size(), 1, "the original atomic write request must
not be batched");
- dsn::message_ex *new_request = nullptr;
- const int err = _app->make_idempotent(request, &new_request);
- if (dsn_unlikely(err != rocksdb::Status::kOk)) {
+ std::vector<dsn::message_ex *> new_requests;
+ const int err = _app->make_idempotent(request, new_requests);
+ if (err != rocksdb::Status::kOk) {
Review Comment:
why remove `dsn_unlikely`?
--
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-unsubscr...@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@pegasus.apache.org
For additional commands, e-mail: dev-h...@pegasus.apache.org
