You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brpc.apache.org by GitBox <gi...@apache.org> on 2020/05/29 02:23:02 UTC
[GitHub] [incubator-brpc] liumh8 opened a new pull request #1128: fix redis args
liumh8 opened a new pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r434985516
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -189,7 +189,9 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- ctx->arena.clear();
+ if (err != PARSE_ERROR_NOT_ENOUGH_DATA) {
+ ctx->arena.clear();
+ }
Review comment:
为什么会每条消息都返回PARSE_ERROR_NOT_ENOUGH_DATA？如果PARSE_ERROR_ABSOLUTELY_WRONG或者PARSE_ERROR_TRY_OTHERS，就会释放了吧
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436566453
##########
File path: src/brpc/redis.cpp
##########
@@ -437,19 +437,21 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
}
bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
Review comment:
已修改
##########
File path: src/brpc/redis.h
##########
@@ -224,11 +225,13 @@ class RedisService {
bool AddCommandHandler(const std::string& name, RedisCommandHandler* handler);
// This function should not be touched by user and used by brpc deverloper only.
- RedisCommandHandler* FindCommandHandler(const std::string& name) const;
+ RedisCommandHandler* FindCommandHandler(const butil::StringPiece& name) const;
private:
- typedef std::unordered_map<std::string, RedisCommandHandler*> CommandMap;
+ typedef BUTIL_HASH_NAMESPACE::hash<butil::StringPiece> StringPieceHasher;
+ typedef std::unordered_map<butil::StringPiece, RedisCommandHandler*, StringPieceHasher> CommandMap;
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r434983839
##########
File path: src/brpc/redis.cpp
##########
@@ -437,19 +437,21 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
}
bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+ butil::StringPiece name_piece(name);
+ auto it = _command_map.find(name_piece);
if (it != _command_map.end()) {
LOG(ERROR) << "redis command name=" << name << " exist";
return false;
}
- _command_map[lcname] = handler;
+
+ _all_commands.push_back(name);
+ name_piece = _all_commands.back();
Review comment:
这个是因为FindCommandHandler要通过StringPiece查找Handler，`typedef std::unordered_map<butil::StringPiece, RedisCommandHandler*, StringPieceHasher> CommandMap` 所以这个map存的是StringPiece，为了使这个StringPiece所指向的字符串内存没有被释放，所以用了个std::list<string\>来保存
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 closed pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 closed pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r434983839
##########
File path: src/brpc/redis.cpp
##########
@@ -437,19 +437,21 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
}
bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+ butil::StringPiece name_piece(name);
+ auto it = _command_map.find(name_piece);
if (it != _command_map.end()) {
LOG(ERROR) << "redis command name=" << name << " exist";
return false;
}
- _command_map[lcname] = handler;
+
+ _all_commands.push_back(name);
+ name_piece = _all_commands.back();
Review comment:
这个是因为FindCommandHandler要通过StringPiece查找Handler，`typedef std::unordered_map<butil::StringPiece, RedisCommandHandler*, StringPieceHasher> CommandMap` 所以这个map存的是StringPiece，为了使这个StringPiece所指向的字符串内存没有被释放，所以用了个std::list<string>来保存
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
zyearn commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r435052914
##########
File path: src/brpc/redis.cpp
##########
@@ -437,19 +437,21 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
}
bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+ butil::StringPiece name_piece(name);
+ auto it = _command_map.find(name_piece);
if (it != _command_map.end()) {
LOG(ERROR) << "redis command name=" << name << " exist";
return false;
}
- _command_map[lcname] = handler;
+
+ _all_commands.push_back(name);
+ name_piece = _all_commands.back();
+ _command_map[name_piece] = handler;
return true;
}
-RedisCommandHandler* RedisService::FindCommandHandler(const std::string& name) const {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+RedisCommandHandler* RedisService::FindCommandHandler(const butil::StringPiece& name) const {
+ auto it = _command_map.find(name);
Review comment:
这里的StringToLowerASCII好像漏掉了
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 closed pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 closed pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 edited a comment on pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 edited a comment on pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#issuecomment-638591963
> @liumh8 在github上提交一个commit会自动跑ci的，不用先close再reopen
这个是因为跑ci失败了，我这边好像只能reopen才能让它再跑一次。你看一下这几个ci失败，应该不是我的问题？[694222518](https://travis-ci.org/github/apache/incubator-brpc/builds/694222518)
[692861060](https://travis-ci.org/github/apache/incubator-brpc/builds/692861060)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
jamesge commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436568422
##########
File path: src/brpc/redis_command.cpp
##########
@@ -420,7 +420,7 @@ ParseError RedisCommandParser::Consume(butil::IOBuf& buf,
char* d = (char*)arena->allocate((len/8 + 1) * 8);
buf.cutn(d, len);
d[len] = '\0';
- _commands[_index] = d;
+ _args[_index] = butil::StringPiece(d, len);
Review comment:
_args[_index].set(d, len);
##########
File path: src/brpc/redis.cpp
##########
@@ -436,8 +436,9 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
return os;
}
-bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
+bool RedisService::AddCommandHandler(const butil::StringPiece& name,
+ RedisCommandHandler* handler) {
+ std::string lcname = StringToLowerASCII(name.as_string());
Review comment:
不用as_string，本就支持StringPiece。如果还得转成string，参数用StringPiece就没有意义了
##########
File path: src/brpc/redis.cpp
##########
@@ -447,9 +448,8 @@ bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandle
return true;
}
-RedisCommandHandler* RedisService::FindCommandHandler(const std::string& name) const {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+RedisCommandHandler* RedisService::FindCommandHandler(const butil::StringPiece& name) const {
+ auto it = _command_map.find(name.as_string());
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 closed pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 closed pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
jamesge commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436523210
##########
File path: src/brpc/redis.cpp
##########
@@ -437,19 +437,21 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
}
bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
Review comment:
函数参数换成stringpiece就行了，不用对_command_map做改造，后续维护太容易造成内存问题
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
zyearn commented on pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#issuecomment-638590744
@liumh8 在github上提交一个commit会自动跑ci的，不用先close再reopen
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
zyearn commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r433021045
##########
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:
好的，主要是因为example是大家都会复制粘贴的，所以里面的代码尽量要做到efficient & readable.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436578532
##########
File path: src/brpc/redis.cpp
##########
@@ -447,9 +448,8 @@ bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandle
return true;
}
-RedisCommandHandler* RedisService::FindCommandHandler(const std::string& name) const {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+RedisCommandHandler* RedisService::FindCommandHandler(const butil::StringPiece& name) const {
+ auto it = _command_map.find(name.as_string());
Review comment:
https://github.com/apache/incubator-brpc/pull/1128#discussion_r435053827
在parse的时候已经转小写了
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 closed pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 closed pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r432825252
##########
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:
这个是为了方便，不修改后面的逻辑。
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r435774755
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -189,7 +189,9 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- ctx->arena.clear();
+ if (err != PARSE_ERROR_NOT_ENOUGH_DATA) {
+ ctx->arena.clear();
+ }
Review comment:
我先把这个修改去掉了，修复这个问题涉及到了之前说的`flush_batch`的问题，你们后续尽快一同修复一下这两个问题吧？这个pull request就只针对'\0'和空串的问题。
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
jamesge commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436529673
##########
File path: src/brpc/redis.h
##########
@@ -224,11 +225,13 @@ class RedisService {
bool AddCommandHandler(const std::string& name, RedisCommandHandler* handler);
// This function should not be touched by user and used by brpc deverloper only.
- RedisCommandHandler* FindCommandHandler(const std::string& name) const;
+ RedisCommandHandler* FindCommandHandler(const butil::StringPiece& name) const;
private:
- typedef std::unordered_map<std::string, RedisCommandHandler*> CommandMap;
+ typedef BUTIL_HASH_NAMESPACE::hash<butil::StringPiece> StringPieceHasher;
+ typedef std::unordered_map<butil::StringPiece, RedisCommandHandler*, StringPieceHasher> CommandMap;
Review comment:
这依赖于std::list的内存管理，如果后续不知情的人换成vector，在resize后其中元素的地址可能就都变了，从而引发bug。另外，这里的“存"其实不是瓶颈，用string就行了（除非是极度热点的地方），查目前也可以用string，以后觉得是热点再换不迟。
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
zyearn commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r435045025
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -189,7 +189,9 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- ctx->arena.clear();
+ if (err != PARSE_ERROR_NOT_ENOUGH_DATA) {
+ ctx->arena.clear();
+ }
Review comment:
第一个请求发了1.5条消息，解析了第一条，第二条解析了一半分配在arena上，之后的请求又只解析了一条完整的，剩下的在arena上，以此往复。
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
zyearn commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r434980490
##########
File path: src/brpc/redis.cpp
##########
@@ -437,19 +437,21 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
}
bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+ butil::StringPiece name_piece(name);
+ auto it = _command_map.find(name_piece);
if (it != _command_map.end()) {
LOG(ERROR) << "redis command name=" << name << " exist";
return false;
}
- _command_map[lcname] = handler;
+
+ _all_commands.push_back(name);
+ name_piece = _all_commands.back();
Review comment:
这个_all_commands有什么用途吗
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -189,7 +189,9 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- ctx->arena.clear();
+ if (err != PARSE_ERROR_NOT_ENOUGH_DATA) {
+ ctx->arena.clear();
+ }
Review comment:
这样会不会出现，每条消息都返回PARSE_ERROR_NOT_ENOUGH_DATA，导致这个链接上的arena一直不释放
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436514579
##########
File path: src/brpc/redis.h
##########
@@ -224,11 +225,13 @@ class RedisService {
bool AddCommandHandler(const std::string& name, RedisCommandHandler* handler);
// This function should not be touched by user and used by brpc deverloper only.
- RedisCommandHandler* FindCommandHandler(const std::string& name) const;
+ RedisCommandHandler* FindCommandHandler(const butil::StringPiece& name) const;
private:
- typedef std::unordered_map<std::string, RedisCommandHandler*> CommandMap;
+ typedef BUTIL_HASH_NAMESPACE::hash<butil::StringPiece> StringPieceHasher;
+ typedef std::unordered_map<butil::StringPiece, RedisCommandHandler*, StringPieceHasher> CommandMap;
Review comment:
StringPiece指向的string保存在了`std::list<std::string> _all_commands`，这样会有问题吗
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r434983839
##########
File path: src/brpc/redis.cpp
##########
@@ -437,19 +437,21 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
}
bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+ butil::StringPiece name_piece(name);
+ auto it = _command_map.find(name_piece);
if (it != _command_map.end()) {
LOG(ERROR) << "redis command name=" << name << " exist";
return false;
}
- _command_map[lcname] = handler;
+
+ _all_commands.push_back(name);
+ name_piece = _all_commands.back();
Review comment:
这个是因为FindCommandHandler要通过StringPiece查找Handler，`typedef std::unordered_map<butil::StringPiece, RedisCommandHandler*, StringPieceHasher> CommandMap` 所以这个map存的是StringPiece，为了使这个StringPiece所指向的字符串内存没有被释放，所以用了个list<string>来保存
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r434985516
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -189,7 +189,9 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- ctx->arena.clear();
+ if (err != PARSE_ERROR_NOT_ENOUGH_DATA) {
+ ctx->arena.clear();
+ }
Review comment:
为什么会每条消息都返回PARSE_ERROR_NOT_ENOUGH_DATA？不会一直都是PARSE_ERROR_NOT_ENOUGH_DATA吧，如果PARSE_ERROR_ABSOLUTELY_WRONG或者PARSE_ERROR_TRY_OTHERS，就会释放了吧
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r434986701
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -189,7 +189,9 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- ctx->arena.clear();
+ if (err != PARSE_ERROR_NOT_ENOUGH_DATA) {
+ ctx->arena.clear();
+ }
Review comment:
> 为什么会每条消息都返回PARSE_ERROR_NOT_ENOUGH_DATA？不会一直都是PARSE_ERROR_NOT_ENOUGH_DATA吧，如果PARSE_ERROR_ABSOLUTELY_WRONG或者PARSE_ERROR_TRY_OTHERS，就会释放了吧
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r432825281
##########
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:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r432825408
##########
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:
如果中间包含'\0'就不能完全得比较了。
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r434527352
##########
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());
Review comment:
已修改
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -76,7 +76,7 @@ class RedisConnContext : public Destroyable {
};
int ConsumeCommand(RedisConnContext* ctx,
- const std::vector<const char*>& commands,
+ const std::vector<butil::StringPiece>& commands,
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jamesge merged pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
jamesge merged pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r434985516
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -189,7 +189,9 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- ctx->arena.clear();
+ if (err != PARSE_ERROR_NOT_ENOUGH_DATA) {
+ ctx->arena.clear();
+ }
Review comment:
为什么会每条消息都返回PARSE_ERROR_NOT_ENOUGH_DATA？
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
jamesge commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436590301
##########
File path: src/brpc/redis.cpp
##########
@@ -447,9 +448,8 @@ bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandle
return true;
}
-RedisCommandHandler* RedisService::FindCommandHandler(const std::string& name) const {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+RedisCommandHandler* RedisService::FindCommandHandler(const butil::StringPiece& name) const {
+ auto it = _command_map.find(name.as_string());
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r433019890
##########
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:
已修改
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 closed pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 closed pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r435053827
##########
File path: src/brpc/redis.cpp
##########
@@ -437,19 +437,21 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
}
bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+ butil::StringPiece name_piece(name);
+ auto it = _command_map.find(name_piece);
if (it != _command_map.end()) {
LOG(ERROR) << "redis command name=" << name << " exist";
return false;
}
- _command_map[lcname] = handler;
+
+ _all_commands.push_back(name);
+ name_piece = _all_commands.back();
+ _command_map[name_piece] = handler;
return true;
}
-RedisCommandHandler* RedisService::FindCommandHandler(const std::string& name) const {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+RedisCommandHandler* RedisService::FindCommandHandler(const butil::StringPiece& name) const {
+ auto it = _command_map.find(name);
Review comment:
https://github.com/apache/incubator-brpc/blob/aba2676d4be6bb713c1977ae1855ba55d2b26b1b/src/brpc/redis_command.cpp#L427
这里已经有了，应该不需要了吧？
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#issuecomment-638591963
> @liumh8 在github上提交一个commit会自动跑ci的，不用先close再reopen
这个是因为跑ci失败了，我这边好像只能reopen才能让它再跑一次。你看一下这几个ci失败，应该不是我的问题？https://travis-ci.org/github/apache/incubator-brpc/builds/694222518，https://travis-ci.org/github/apache/incubator-brpc/builds/692861060
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
zyearn commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r435057587
##########
File path: src/brpc/redis.cpp
##########
@@ -437,19 +437,21 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
}
bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+ butil::StringPiece name_piece(name);
+ auto it = _command_map.find(name_piece);
if (it != _command_map.end()) {
LOG(ERROR) << "redis command name=" << name << " exist";
return false;
}
- _command_map[lcname] = handler;
+
+ _all_commands.push_back(name);
+ name_piece = _all_commands.back();
+ _command_map[name_piece] = handler;
return true;
}
-RedisCommandHandler* RedisService::FindCommandHandler(const std::string& name) const {
- std::string lcname = StringToLowerASCII(name);
- auto it = _command_map.find(lcname);
+RedisCommandHandler* RedisService::FindCommandHandler(const butil::StringPiece& name) const {
+ auto it = _command_map.find(name);
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
jamesge commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436591336
##########
File path: src/brpc/redis.cpp
##########
@@ -436,8 +436,9 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
return os;
}
-bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
+bool RedisService::AddCommandHandler(const butil::StringPiece& name,
+ RedisCommandHandler* handler) {
+ std::string lcname = StringToLowerASCII(name.as_string());
Review comment:
行，你先改回string吧，这个点我之后改一下
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436583876
##########
File path: src/brpc/redis.cpp
##########
@@ -436,8 +436,9 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
return os;
}
-bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
+bool RedisService::AddCommandHandler(const butil::StringPiece& name,
+ RedisCommandHandler* handler) {
+ std::string lcname = StringToLowerASCII(name.as_string());
Review comment:
StringToLowerASCII不支持StringPiece吧？StringPiece里只有const_iterator，没有iterator。其实这个函数参数用string就好了吧？
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r433039156
##########
File path: example/redis_c++/redis_server.cpp
##########
@@ -64,14 +64,14 @@ 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,
+ brpc::RedisReply* output,
+ bool flush_batched) override {
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r435063188
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -189,7 +189,9 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- ctx->arena.clear();
+ if (err != PARSE_ERROR_NOT_ENOUGH_DATA) {
+ ctx->arena.clear();
+ }
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
zyearn commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r435827257
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -189,7 +189,9 @@ ParseResult ParseRedisMessage(butil::IOBuf* source, Socket* socket,
wopt.ignore_eovercrowded = true;
LOG_IF(WARNING, socket->Write(&sendbuf, &wopt) != 0)
<< "Fail to send redis reply";
- ctx->arena.clear();
+ if (err != PARSE_ERROR_NOT_ENOUGH_DATA) {
+ ctx->arena.clear();
+ }
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] zyearn commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
zyearn commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r433033495
##########
File path: example/redis_c++/redis_server.cpp
##########
@@ -64,14 +64,14 @@ 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,
+ brpc::RedisReply* output,
+ bool flush_batched) override {
Review comment:
这里把flush_batched的注释删掉了？
##########
File path: example/redis_c++/redis_server.cpp
##########
@@ -90,15 +90,15 @@ class SetCommandHandler : public brpc::RedisCommandHandler {
explicit SetCommandHandler(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,
+ brpc::RedisReply* output,
+ bool flush_batched) override {
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 closed pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 closed pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 closed pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 closed pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
jamesge commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r433104470
##########
File path: src/brpc/policy/redis_protocol.cpp
##########
@@ -76,7 +76,7 @@ class RedisConnContext : public Destroyable {
};
int ConsumeCommand(RedisConnContext* ctx,
- const std::vector<const char*>& commands,
+ const std::vector<butil::StringPiece>& commands,
Review comment:
这是args还是commands？
##########
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());
Review comment:
FindCommandHandler的参数可以改成StringPiece？
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436636520
##########
File path: src/brpc/redis.cpp
##########
@@ -436,8 +436,9 @@ std::ostream& operator<<(std::ostream& os, const RedisResponse& response) {
return os;
}
-bool RedisService::AddCommandHandler(const std::string& name, RedisCommandHandler* handler) {
- std::string lcname = StringToLowerASCII(name);
+bool RedisService::AddCommandHandler(const butil::StringPiece& name,
+ RedisCommandHandler* handler) {
+ std::string lcname = StringToLowerASCII(name.as_string());
Review comment:
已修改
##########
File path: src/brpc/redis_command.cpp
##########
@@ -420,7 +420,7 @@ ParseError RedisCommandParser::Consume(butil::IOBuf& buf,
char* d = (char*)arena->allocate((len/8 + 1) * 8);
buf.cutn(d, len);
d[len] = '\0';
- _commands[_index] = d;
+ _args[_index] = butil::StringPiece(d, len);
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] jamesge commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
jamesge commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r436511740
##########
File path: src/brpc/redis.h
##########
@@ -224,11 +225,13 @@ class RedisService {
bool AddCommandHandler(const std::string& name, RedisCommandHandler* handler);
// This function should not be touched by user and used by brpc deverloper only.
- RedisCommandHandler* FindCommandHandler(const std::string& name) const;
+ RedisCommandHandler* FindCommandHandler(const butil::StringPiece& name) const;
private:
- typedef std::unordered_map<std::string, RedisCommandHandler*> CommandMap;
+ typedef BUTIL_HASH_NAMESPACE::hash<butil::StringPiece> StringPieceHasher;
+ typedef std::unordered_map<butil::StringPiece, RedisCommandHandler*, StringPieceHasher> CommandMap;
Review comment:
StringPiece肯定不能做key，内存管理有问题。
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 closed pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 closed pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r432825430
##########
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:
如果中间包含'\0'就不能完全得比较了
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org
[GitHub] [incubator-brpc] liumh8 commented on a change in pull request #1128: fix redis args
Posted by GitBox <gi...@apache.org>.
liumh8 commented on a change in pull request #1128:
URL: https://github.com/apache/incubator-brpc/pull/1128#discussion_r433039156
##########
File path: example/redis_c++/redis_server.cpp
##########
@@ -64,14 +64,14 @@ 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,
+ brpc::RedisReply* output,
+ bool flush_batched) override {
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@brpc.apache.org
For additional commands, e-mail: dev-help@brpc.apache.org