zyearn commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r432409539
##########
File path: example/redis_c++/redis_server.cpp
##########
@@ -64,9 +64,13 @@ class GetCommandHandler : public brpc::RedisCommandHandler {
explicit GetCommandHandler(RedisServiceImpl* rsimpl)
: _rsimpl(rsimpl) {}
- brpc::RedisCommandHandlerResult Run(const std::vector<const char*>& args,
- brpc::RedisReply* output,
- bool /*flush_batched*/) override {
+ brpc::RedisCommandHandlerResult Run(const std::vector<butil::StringPiece>&
args_piece,
+ brpc::RedisReply* output,
+ bool flush_batched) override {
+ std::vector<std::string> args;
+ for (size_t i = 0; i < args_piece.size(); ++i) {
+ args.emplace_back(args_piece[i].data(), args_piece.size());
+ }
Review comment:
这边为什么要拷贝一份出来呢
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -90,10 +90,10 @@ int ConsumeCommand(RedisConnContext* ctx,
return -1;
}
} else {
- RedisCommandHandler* ch =
ctx->redis_service->FindCommandHandler(commands[0]);
+ RedisCommandHandler* ch =
ctx->redis_service->FindCommandHandler(commands[0].as_string());
if (!ch) {
char buf[64];
- snprintf(buf, sizeof(buf), "ERR unknown command `%s`",
commands[0]);
+ snprintf(buf, sizeof(buf), "ERR unknown command `%s`",
commands[0].data());
Review comment:
这边data()不能保证是\0结尾的，所以会多打一些字符
##########
File path: test/brpc_redis_unittest.cpp
##########
@@ -900,9 +904,13 @@ class GetCommandHandler : public brpc::RedisCommandHandler
{
: _rs(rs)
, _batch_process(batch_process) {}
- brpc::RedisCommandHandlerResult Run(const std::vector<const char*>& args,
- brpc::RedisReply* output,
- bool flush_batched) {
+ brpc::RedisCommandHandlerResult Run(const std::vector<butil::StringPiece>&
args_piece,
+ brpc::RedisReply* output,
+ bool flush_batched) {
+ std::vector<std::string> args;
+ for (size_t i = 0; i < args_piece.size(); ++i) {
+ args.emplace_back(args_piece[i].data(), args_piece[i].size());
+ }
Review comment:
同上
##########
File path: test/brpc_redis_unittest.cpp
##########
@@ -565,15 +565,15 @@ std::string GetCompleteCommand(const std::vector<const
char*>& commands) {
TEST_F(RedisTest, command_parser) {
brpc::RedisCommandParser parser;
butil::IOBuf buf;
- std::vector<const char*> command_out;
+ std::vector<butil::StringPiece> command_out;
butil::Arena arena;
{
// parse from whole command
std::string command = "set abc edc";
ASSERT_TRUE(brpc::RedisCommandNoFormat(&buf, command.c_str()).ok());
ASSERT_EQ(brpc::PARSE_OK, parser.Consume(buf, &command_out, &arena));
ASSERT_TRUE(buf.empty());
- ASSERT_STREQ(command.c_str(), GetCompleteCommand(command_out).c_str());
+ ASSERT_EQ(command, GetCompleteCommand(command_out));
Review comment:
为什么要改成ASSERT_EQ?
##########
File path: test/brpc_redis_unittest.cpp
##########
@@ -869,9 +869,13 @@ class SetCommandHandler : public brpc::RedisCommandHandler
{
: _rs(rs)
, _batch_process(batch_process) {}
- brpc::RedisCommandHandlerResult Run(const std::vector<const char*>& args,
- brpc::RedisReply* output,
- bool flush_batched) {
+ brpc::RedisCommandHandlerResult Run(const std::vector<butil::StringPiece>&
args_piece,
+ brpc::RedisReply* output,
+ bool flush_batched) {
+ std::vector<std::string> args;
+ for (size_t i = 0; i < args_piece.size(); ++i) {
+ args.emplace_back(args_piece[i].data(), args_piece[i].size());
+ }
Review comment:
同上，这边为什么要拷贝一份出来呢
##########
File path: src/brpc/redis_command.cpp
##########
@@ -389,7 +389,7 @@ ParseError RedisCommandParser::Consume(butil::IOBuf& buf,
LOG(ERROR) << '`' << intbuf + 1 << "' is not a valid 64-bit decimal";
return PARSE_ERROR_ABSOLUTELY_WRONG;
}
- if (value <= 0) {
+ if (value < 0) {
Review comment:
可以给value==0的情况加个单测吗
##########
File path: test/brpc_redis_unittest.cpp
##########
@@ -593,7 +593,7 @@ TEST_F(RedisTest, command_parser) {
}
}
ASSERT_TRUE(buf.empty());
- ASSERT_STREQ(GetCompleteCommand(command_out).c_str(), "set abc
def");
+ ASSERT_EQ(GetCompleteCommand(command_out), "set abc def");
Review comment:
同上
##########
File path: test/brpc_redis_unittest.cpp
##########
@@ -128,7 +128,7 @@ void AssertReplyEqual(const brpc::RedisReply& reply1,
// fall through
case brpc::REDIS_REPLY_STATUS:
ASSERT_NE(reply1.c_str(), reply2.c_str()); // from different arena
- ASSERT_STREQ(reply1.c_str(), reply2.c_str());
+ ASSERT_EQ(reply1.data(), reply2.data());
Review comment:
为什么要改成ASSERT_EQ?
##########
File path: test/brpc_redis_unittest.cpp
##########
@@ -933,9 +941,13 @@ class IncrCommandHandler : public
brpc::RedisCommandHandler {
public:
IncrCommandHandler() {}
- brpc::RedisCommandHandlerResult Run(const std::vector<const char*>& args,
- brpc::RedisReply* output,
- bool flush_batched) {
+ brpc::RedisCommandHandlerResult Run(const std::vector<butil::StringPiece>&
args_piece,
+ brpc::RedisReply* output,
+ bool flush_batched) {
+ std::vector<std::string> args;
+ for (size_t i = 0; i < args_piece.size(); ++i) {
+ args.emplace_back(args_piece[i].data(), args_piece[i].size());
+ }
Review comment:
同上
##########
File path: test/brpc_redis_unittest.cpp
##########
@@ -1060,14 +1072,18 @@ class MultiCommandHandler : public
brpc::RedisCommandHandler {
class MultiTransactionHandler : public brpc::RedisCommandHandler {
public:
- brpc::RedisCommandHandlerResult Run(const std::vector<const char*>&
args,
- brpc::RedisReply* output,
- bool flush_batched) {
- if (strcmp(args[0], "multi") == 0) {
+ brpc::RedisCommandHandlerResult Run(const
std::vector<butil::StringPiece>& args_piece,
+ brpc::RedisReply* output,
+ bool flush_batched) {
+ std::vector<std::string> args;
+ for (size_t i = 0; i < args_piece.size(); ++i) {
+ args.emplace_back(args_piece[i].data(), args_piece[i].size());
+ }
Review 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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
