You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "infdahai (via GitHub)" <gi...@apache.org> on 2023/06/27 15:03:00 UTC
[GitHub] [kvrocks] infdahai opened a new pull request, #1531: Add some preset comments for list commands.
infdahai opened a new pull request, #1531:
URL: https://github.com/apache/kvrocks/pull/1531
Based on [the comment](https://github.com/apache/kvrocks/issues/1512#issuecomment-1609624027_), we add some preset comments to specific the location for the submitters with working on #1512.
--
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] [kvrocks] torwig commented on a diff in pull request #1531: Add some preset comments for list commands.
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#discussion_r1243943148
##########
src/commands/cmd_list.cc:
##########
@@ -551,19 +551,22 @@ class CommandLMove : public Commander {
bool dst_left_;
};
-REDIS_REGISTER_COMMANDS(
- MakeCmdAttr<CommandLPush>("lpush", -3, "write", 1, 1, 1), MakeCmdAttr<CommandRPush>("rpush", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1, 1, 1), MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPop>("rpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandLRem>("lrem", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
- MakeCmdAttr<CommandLRange>("lrange", 4, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLTrim>("ltrim", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLSet>("lset", 4, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPopLPUSH>("rpoplpush", 3, "write", 1, 2, 1),
- MakeCmdAttr<CommandLMove>("lmove", 5, "write", 1, 2, 1), )
+REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
+ MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
+ MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1), //
Review Comment:
What is the purpose of the `//` at the end of the line?
--
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] [kvrocks] torwig commented on a diff in pull request #1531: Add some preset comments for list commands.
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#discussion_r1243960533
##########
src/commands/cmd_list.cc:
##########
@@ -551,19 +551,22 @@ class CommandLMove : public Commander {
bool dst_left_;
};
-REDIS_REGISTER_COMMANDS(
- MakeCmdAttr<CommandLPush>("lpush", -3, "write", 1, 1, 1), MakeCmdAttr<CommandRPush>("rpush", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1, 1, 1), MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPop>("rpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandLRem>("lrem", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
- MakeCmdAttr<CommandLRange>("lrange", 4, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLTrim>("ltrim", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLSet>("lset", 4, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPopLPUSH>("rpoplpush", 3, "write", 1, 2, 1),
- MakeCmdAttr<CommandLMove>("lmove", 5, "write", 1, 2, 1), )
+REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
+ MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
+ MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1),
+ MakeCmdAttr<CommandLMove>("lmove", 5, "write", 1, 2, 1),
+ MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1), //
Review Comment:
@infdahai Thank you for the explanation.
--
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] [kvrocks] infdahai commented on a diff in pull request #1531: Add some preset comments for list commands.
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#discussion_r1243955828
##########
src/commands/cmd_list.cc:
##########
@@ -551,19 +551,22 @@ class CommandLMove : public Commander {
bool dst_left_;
};
-REDIS_REGISTER_COMMANDS(
- MakeCmdAttr<CommandLPush>("lpush", -3, "write", 1, 1, 1), MakeCmdAttr<CommandRPush>("rpush", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1, 1, 1), MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPop>("rpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandLRem>("lrem", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
- MakeCmdAttr<CommandLRange>("lrange", 4, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLTrim>("ltrim", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLSet>("lset", 4, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPopLPUSH>("rpoplpush", 3, "write", 1, 2, 1),
- MakeCmdAttr<CommandLMove>("lmove", 5, "write", 1, 2, 1), )
+REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
+ MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
+ MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1),
+ MakeCmdAttr<CommandLMove>("lmove", 5, "write", 1, 2, 1),
+ MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1), //
Review Comment:
This one will allow the format to be maintained. If I remove this, the code will merge together. And the commands are arranged in the order of the redis documentation
--
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] [kvrocks] infdahai commented on a diff in pull request #1531: Add some preset comments for list commands.
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#discussion_r1243927390
##########
src/commands/cmd_list.cc:
##########
@@ -556,14 +556,19 @@ REDIS_REGISTER_COMMANDS(
MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1, 1, 1),
MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1, 1, 1), MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),
Review Comment:
https://github.com/infdahai/incubator-kvrocks/blob/aab1cf9784eb9e271408d04c45d991c54b1aa9aa/src/commands/cmd_list.cc#L552-L568
How about this?
--
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] [kvrocks] infdahai commented on a diff in pull request #1531: Add some preset comments for list commands.
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#discussion_r1243949095
##########
src/commands/cmd_list.cc:
##########
@@ -551,19 +551,22 @@ class CommandLMove : public Commander {
bool dst_left_;
};
-REDIS_REGISTER_COMMANDS(
- MakeCmdAttr<CommandLPush>("lpush", -3, "write", 1, 1, 1), MakeCmdAttr<CommandRPush>("rpush", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1, 1, 1), MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPop>("rpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandLRem>("lrem", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
- MakeCmdAttr<CommandLRange>("lrange", 4, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLTrim>("ltrim", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLSet>("lset", 4, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPopLPUSH>("rpoplpush", 3, "write", 1, 2, 1),
- MakeCmdAttr<CommandLMove>("lmove", 5, "write", 1, 2, 1), )
+REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
+ MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
+ MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1), //
Review Comment:
You are right. I remove this.
--
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] [kvrocks] infdahai commented on pull request #1531: Add some preset comments for list commands.
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#issuecomment-1609741805
No changes to these commands, just a change of order to make the format reasonable.
--
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] [kvrocks] torwig commented on a diff in pull request #1531: Add some preset comments for list commands.
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#discussion_r1243951075
##########
src/commands/cmd_list.cc:
##########
@@ -551,19 +551,22 @@ class CommandLMove : public Commander {
bool dst_left_;
};
-REDIS_REGISTER_COMMANDS(
- MakeCmdAttr<CommandLPush>("lpush", -3, "write", 1, 1, 1), MakeCmdAttr<CommandRPush>("rpush", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1, 1, 1), MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPop>("rpop", -2, "write", 1, 1, 1),
- MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
- MakeCmdAttr<CommandLRem>("lrem", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
- MakeCmdAttr<CommandLRange>("lrange", 4, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLTrim>("ltrim", 4, "write", 1, 1, 1), MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1),
- MakeCmdAttr<CommandLSet>("lset", 4, "write", 1, 1, 1),
- MakeCmdAttr<CommandRPopLPUSH>("rpoplpush", 3, "write", 1, 2, 1),
- MakeCmdAttr<CommandLMove>("lmove", 5, "write", 1, 2, 1), )
+REDIS_REGISTER_COMMANDS(MakeCmdAttr<CommandBLPop>("blpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandBRPop>("brpop", -3, "write no-script", 1, -2, 1),
+ MakeCmdAttr<CommandLIndex>("lindex", 3, "read-only", 1, 1, 1),
+ MakeCmdAttr<CommandLInsert>("linsert", 5, "write", 1, 1, 1),
+ MakeCmdAttr<CommandLLen>("llen", 2, "read-only", 1, 1, 1),
+ MakeCmdAttr<CommandLMove>("lmove", 5, "write", 1, 2, 1),
+ MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1), //
Review Comment:
Still one is present :)
--
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] [kvrocks] torwig commented on a diff in pull request #1531: Add some preset comments for list commands.
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#discussion_r1243932988
##########
src/commands/cmd_list.cc:
##########
@@ -556,14 +556,19 @@ REDIS_REGISTER_COMMANDS(
MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1, 1, 1),
MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1, 1, 1), MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),
Review Comment:
Yes, it works for me. Personally, I think comments like `// impl the X command here` isn't necessary - it explains nothing and actually is useless.
--
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] [kvrocks] infdahai commented on a diff in pull request #1531: Add some preset comments for list commands.
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#discussion_r1243927390
##########
src/commands/cmd_list.cc:
##########
@@ -556,14 +556,19 @@ REDIS_REGISTER_COMMANDS(
MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1, 1, 1),
MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1, 1, 1), MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),
Review Comment:
https://github.com/infdahai/incubator-kvrocks/blob/aab1cf9784eb9e271408d04c45d991c54b1aa9aa/src/commands/cmd_list.cc#L552-L68
How about this?
--
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] [kvrocks] torwig commented on a diff in pull request #1531: Add some preset comments for list commands.
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531#discussion_r1243916418
##########
src/commands/cmd_list.cc:
##########
@@ -556,14 +556,19 @@ REDIS_REGISTER_COMMANDS(
MakeCmdAttr<CommandLPushX>("lpushx", -3, "write", 1, 1, 1),
MakeCmdAttr<CommandRPushX>("rpushx", -3, "write", 1, 1, 1), MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),
Review Comment:
Could you also move `MakeCmdAttr<CommandLPop>("lpop", -2, "write", 1, 1, 1),` to a new line? And others to have a clear list of 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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kvrocks] git-hulk merged pull request #1531: Arrange some register comments for list commands.
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #1531:
URL: https://github.com/apache/kvrocks/pull/1531
--
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