You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "wy-ei (via GitHub)" <gi...@apache.org> on 2023/05/27 05:52:43 UTC
[GitHub] [incubator-kvrocks] wy-ei opened a new pull request, #1476: fix bad case in the format of monitor response and improve performance
wy-ei opened a new pull request, #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476
## fix the format error in monitor response
If the payload of a request has `\r\n`, the response forwarded to monitoring client is in bad format since simple string can't contains CRLF in RESP.
I found this issue when I execute monitor command in redis-cli and set content of a `.c` file as value in another redis-cli:
```c
$ redis-cli -p 6666
127.0.0.1:6666> monitor
OK
1685165694.988910 [__namespace 127.0.0.1:55614] "set" "a" "#include <stdio.h>
Error: Protocol error, got "\r" as reply type byte
```
```
$ redis-cli -p 6666 -x set a < ./main.c
```
In the redis code the monitor response sending to client has been escaped by `sdscatrepr`:
https://github.com/redis/redis/blob/e775b34e813654ead5be899faa065f1c31753040/src/replication.c#L560
https://github.com/redis/redis/blob/e775b34e813654ead5be899faa065f1c31753040/src/sds.c#L986
I implement a function `StringRepr` in `string_util` to do the escap which did the same escape with `sdscatrepr`.
## improve the performance of monitor
the content of monitor response is formated in worker, if more than one worker exists, the content will be formated several times.
We can format the response in `Server::FeedMonitorConns` and pass the response to `Worker::FeedMonitorConns`.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1476: fix bad case in the format of monitor response and improve performance
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476#issuecomment-1565224719
@wy-ei Cool, thanks
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1476: fix bad case in the format of monitor response and improve performance
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476#discussion_r1209708213
##########
src/common/string_util.cc:
##########
@@ -312,4 +314,47 @@ std::vector<std::string> TokenizeRedisProtocol(const std::string &value) {
return tokens;
}
+/* convert string to an escaped representation where
+ * all the non-printable characters (tested with isprint()) are turned into
+ * escapes in the form "\n\r\a...." or "\x<hex-number>". */
+std::string StringRepr(const std::string &s) {
Review Comment:
Thanks
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] wy-ei commented on a diff in pull request #1476: fix bad case in the format of monitor response and improve performance
Posted by "wy-ei (via GitHub)" <gi...@apache.org>.
wy-ei commented on code in PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476#discussion_r1209702432
##########
src/common/string_util.cc:
##########
@@ -312,4 +314,47 @@ std::vector<std::string> TokenizeRedisProtocol(const std::string &value) {
return tokens;
}
+/* convert string to an escaped representation where
+ * all the non-printable characters (tested with isprint()) are turned into
+ * escapes in the form "\n\r\a...." or "\x<hex-number>". */
+std::string StringRepr(const std::string &s) {
Review Comment:
@git-hulk function name was changed, and test case was added.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1476: fix bad case in the format of monitor response and improve performance
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476#issuecomment-1565226927
IMHO I think maybe we should just use bulk string reply rather than simple string to solve this problem, and these string escaping is just like a workaround.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on pull request #1476: Escape the special chars in the monitor command output
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476#issuecomment-1567893943
Thanks all, merging...
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] mapleFU commented on a diff in pull request #1476: fix bad case in the format of monitor response and improve performance
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476#discussion_r1209216112
##########
src/common/string_util.cc:
##########
@@ -312,4 +314,47 @@ std::vector<std::string> TokenizeRedisProtocol(const std::string &value) {
return tokens;
}
+/* convert string to an escaped representation where
+ * all the non-printable characters (tested with isprint()) are turned into
+ * escapes in the form "\n\r\a...." or "\x<hex-number>". */
+std::string StringRepr(const std::string &s) {
+ std::string str;
+ str.reserve(s.size());
+
+ for (auto ch : s) {
+ switch (ch) {
+ case '\\':
+ case '"':
+ str += "\\";
Review Comment:
Would '\' become "\\\"?
##########
src/common/string_util.cc:
##########
@@ -312,4 +314,47 @@ std::vector<std::string> TokenizeRedisProtocol(const std::string &value) {
return tokens;
}
+/* convert string to an escaped representation where
+ * all the non-printable characters (tested with isprint()) are turned into
+ * escapes in the form "\n\r\a...." or "\x<hex-number>". */
+std::string StringRepr(const std::string &s) {
+ std::string str;
+ str.reserve(s.size());
+
+ for (auto ch : s) {
+ switch (ch) {
+ case '\\':
+ case '"':
+ str += "\\";
Review Comment:
Would `'\'` become `"\\\"`?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] wy-ei commented on pull request #1476: fix bad case in the format of monitor response and improve performance
Posted by "wy-ei (via GitHub)" <gi...@apache.org>.
wy-ei commented on PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476#issuecomment-1565235348
@PragmaTwice If redis use bulk string as the monitor response format, this issue does't exists any more. But the simple string is used and the issue is exists. This change is a workaround and it has fix the bad case and make it compatible with redis.
It's easy to change the format as bulk string or array, but client SDK may not work.
For example, in redis-py, unescaping is applyed on monitor response:
https://github.com/redis/redis-py/blob/2d9b5ac6fe03fdc572b8ca47f7134082bae2a5e2/redis/client.py#L1337
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] PragmaTwice commented on pull request #1476: fix bad case in the format of monitor response and improve performance
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476#issuecomment-1565228453
https://redis.io/commands/monitor/
Seems redis itself already has some bad practice in solving these escaping problem, which is sad since we need to be compatible to redis.
For me, escaping is not need at all, we can just send an arrays with bulk strings.
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk merged pull request #1476: Escape the special chars in the monitor command output
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-kvrocks] git-hulk commented on a diff in pull request #1476: fix bad case in the format of monitor response and improve performance
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1476:
URL: https://github.com/apache/incubator-kvrocks/pull/1476#discussion_r1209201953
##########
src/common/string_util.cc:
##########
@@ -312,4 +314,47 @@ std::vector<std::string> TokenizeRedisProtocol(const std::string &value) {
return tokens;
}
+/* convert string to an escaped representation where
+ * all the non-printable characters (tested with isprint()) are turned into
+ * escapes in the form "\n\r\a...." or "\x<hex-number>". */
+std::string StringRepr(const std::string &s) {
Review Comment:
StringRepr => EscapeString
BTW, can you add the cpp test case for this function?
--
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: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org