DCjanus commented on code in PR #2943:
URL: https://github.com/apache/kvrocks/pull/2943#discussion_r2079309335
##########
src/commands/cmd_server.cc:
##########
@@ -497,15 +508,26 @@ class CommandClient : public Commander {
*output = redis::RESP_OK;
}
return Status::OK();
+ } else if (subcommand_ == "reply") {
+ if (reply_mode_arg_ == "on") {
+ conn->SetReplyMode(Connection::ReplyMode::ON);
+ } else if (reply_mode_arg_ == "off") {
+ conn->SetReplyMode(Connection::ReplyMode::OFF);
+ } else if (reply_mode_arg_ == "skip") {
+ conn->SetReplyMode(Connection::ReplyMode::SKIP_ONCE_PENDING);
+ }
+ *output = redis::RESP_OK;
+ return Status::OK();
Review Comment:
Thank you for your suggestion.
> Currently we have 4 states in the `ReplyMode` enum, and the
`SKIP_ONCE_PENDING` seems to ignore the reply of `CLIENT REPLY` itself. But if
we just don't reply in `CLIENT REPLY OFF|SKIP`, maybe we can remove the 3rd
state. e.g.
>
> ```c++
> conn->SetReplyMode(reply_mode_); // reply_mode_ can be ON, OFF or SKIP (3
states).
>
> if (reply_mode_ != Connection::ReplyMode::SKIP) {
> *output = redis::RESP_OK;
> }
> return Status::OK();
> ```
>
> WDYT?
Thank you for your suggestion.
The reason I didn’t write it this way at first is that I wasn’t very
familiar with the codebase. I didn’t realize that leaving `output` empty (not
setting a reply) is actually the expected behavior in this scenario.
I was initially concerned that not setting `output` might cause unexpected
issues. Later, I found that the `Reply` function is only called when `output`
is not empty, so leaving it empty is safe and correct 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
