You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by GitBox <gi...@apache.org> on 2022/10/27 02:23:13 UTC
[GitHub] [incubator-kvrocks] tanruixiang opened a new pull request, #1047: fix: supports multiple rename-commands
tanruixiang opened a new pull request, #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047
This close #1040
--
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 #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#discussion_r1006978648
##########
src/config/config.cc:
##########
@@ -273,23 +273,26 @@ void Config::initFieldValidator() {
}},
{"rename-command",
[](const std::string &k, const std::string &v) -> Status {
- std::vector<std::string> args = Util::Split(v, " \t");
- if (args.size() != 2) {
- return Status(Status::NotOK, "Invalid rename-command format");
- }
- auto commands = Redis::GetCommands();
- auto cmd_iter = commands->find(Util::ToLower(args[0]));
- if (cmd_iter == commands->end()) {
- return Status(Status::NotOK, "No such command in rename-command");
- }
- if (args[1] != "\"\"") {
- auto new_command_name = Util::ToLower(args[1]);
- if (commands->find(new_command_name) != commands->end()) {
- return Status(Status::NotOK, "Target command name already exists");
+ std::vector<std::string> all_args = Util::Split(v, "\n");
Review Comment:
The rename command should not support:
```
rename-command a b
c d
e f
```
so it makes no sense to split with newline here?
##########
src/config/config.cc:
##########
@@ -722,8 +725,15 @@ void Config::Get(std::string key, std::vector<std::string> *values) {
values->clear();
for (const auto &iter : fields_) {
if (key == "*" || Util::ToLower(key) == iter.first) {
- values->emplace_back(iter.first);
- values->emplace_back(iter.second->ToString());
+ if (iter.second->GetConfigType() == configType::MultiConfig) {
Review Comment:
Can you add a test case for this line, it looks not correct for the rename-command now.
--
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 #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
git-hulk merged PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047
--
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 #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#issuecomment-1294706760
Thanks @tanruixiang @PragmaTwice, 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] git-hulk commented on pull request #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
git-hulk commented on PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#issuecomment-1293654416
> https://github.com/apache/incubator-kvrocks/blob/debd8988d6f6978593ad35b7e06d0c3e773ee825/src/config/config.cc#L767-L771
>
> Since **ALL `MultiStringField` cannot be rewritten**, so I think you need to change it to something like
>
> ```
> if (iter->second.IsMulti())
> ```
>
> in case of someone use `MultiStringField` but forget to include the config key in this check.
Yes, that's right.
--
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] tanruixiang commented on a diff in pull request #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#discussion_r1007134298
##########
src/config/config.cc:
##########
@@ -273,23 +273,26 @@ void Config::initFieldValidator() {
}},
{"rename-command",
[](const std::string &k, const std::string &v) -> Status {
- std::vector<std::string> args = Util::Split(v, " \t");
- if (args.size() != 2) {
- return Status(Status::NotOK, "Invalid rename-command format");
- }
- auto commands = Redis::GetCommands();
- auto cmd_iter = commands->find(Util::ToLower(args[0]));
- if (cmd_iter == commands->end()) {
- return Status(Status::NotOK, "No such command in rename-command");
- }
- if (args[1] != "\"\"") {
- auto new_command_name = Util::ToLower(args[1]);
- if (commands->find(new_command_name) != commands->end()) {
- return Status(Status::NotOK, "Target command name already exists");
+ std::vector<std::string> all_args = Util::Split(v, "\n");
Review Comment:
This is because we store the rename-command in the vector, and the code logic is obtained with Tostring. So we used `\n` as delimiter to concat this vector. For example: `KEYS KEYSNEW \n GET GETNEW \n`
--
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 #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#issuecomment-1293650161
https://github.com/apache/incubator-kvrocks/blob/debd8988d6f6978593ad35b7e06d0c3e773ee825/src/config/config.cc#L767-L771
Since ALL `MultiStringField` cannot be rewritten, so I think you need to change it to something like
```
if (iter->second.IsMulti())
```
in case of someone use `MultiStringField` but forget to include the config key in this check.
--
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 a diff in pull request #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#discussion_r1007591934
##########
tests/cppunit/config_test.cc:
##########
@@ -133,6 +133,33 @@ TEST(Config, GetAndSet) {
}
}
+TEST(Config, GetAndSetRenameCommand) {
+ const char *path = "test.conf";
+ unlink(path);
+
+ std::ostringstream string_stream;
+ string_stream << "rename-command KEYS KEYS_NEW"
+ << "\n";
+ string_stream << "rename-command GET GET_NEW"
+ << "\n";
+ string_stream << "rename-command SET SET_NEW"
+ << "\n";
+ std::ofstream output_file(path, std::ios::out);
+ output_file.write(string_stream.str().c_str(), string_stream.str().size());
Review Comment:
You can write something like `output_file << "rename-command KEYS KEYS_NEW\n" << ...` directly.
--
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] tanruixiang commented on a diff in pull request #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
tanruixiang commented on code in PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#discussion_r1007135031
##########
src/config/config.cc:
##########
@@ -722,8 +725,15 @@ void Config::Get(std::string key, std::vector<std::string> *values) {
values->clear();
for (const auto &iter : fields_) {
if (key == "*" || Util::ToLower(key) == iter.first) {
- values->emplace_back(iter.first);
- values->emplace_back(iter.second->ToString());
+ if (iter.second->GetConfigType() == configType::MultiConfig) {
Review Comment:
Test cases have been 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 a diff in pull request #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#discussion_r1006368975
##########
src/config/config.cc:
##########
@@ -723,7 +726,13 @@ void Config::Get(std::string key, std::vector<std::string> *values) {
for (const auto &iter : fields_) {
if (key == "*" || Util::ToLower(key) == iter.first) {
values->emplace_back(iter.first);
- values->emplace_back(iter.second->ToString());
+ if (iter.second->GetConfigType() == configType::MultiConfig) {
+ for (const auto &p : Util::Split(iter.second->ToString(), "\n")) {
+ values->push_back(p);
+ }
+ } else {
+ values->emplace_back(iter.second->ToString());
+ }
Review Comment:
```suggestion
if (iter.second->GetConfigType() == configType::MultiConfig) {
for (const auto &p : Util::Split(iter.second->ToString(), "\n")) {
values->emplace_back(iter.first);
values->push_back(p);
}
} else {
values->emplace_back(iter.first);
values->emplace_back(iter.second->ToString());
}
```
I think we need to keep one-key-one-value pairs, in order to make user to parse the result of `config get *` easily.
##########
src/config/config.cc:
##########
@@ -723,7 +726,13 @@ void Config::Get(std::string key, std::vector<std::string> *values) {
for (const auto &iter : fields_) {
if (key == "*" || Util::ToLower(key) == iter.first) {
values->emplace_back(iter.first);
Review Comment:
```suggestion
```
--
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 a diff in pull request #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
PragmaTwice commented on code in PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#discussion_r1006368975
##########
src/config/config.cc:
##########
@@ -723,7 +726,13 @@ void Config::Get(std::string key, std::vector<std::string> *values) {
for (const auto &iter : fields_) {
if (key == "*" || Util::ToLower(key) == iter.first) {
values->emplace_back(iter.first);
- values->emplace_back(iter.second->ToString());
+ if (iter.second->GetConfigType() == configType::MultiConfig) {
+ for (const auto &p : Util::Split(iter.second->ToString(), "\n")) {
+ values->push_back(p);
+ }
+ } else {
+ values->emplace_back(iter.second->ToString());
+ }
Review Comment:
```suggestion
if (iter.second->GetConfigType() == configType::MultiConfig) {
for (const auto &p : Util::Split(iter.second->ToString(), "\n")) {
values->emplace_back(iter.first);
values->push_back(p);
}
} else {
values->emplace_back(iter.first);
values->emplace_back(iter.second->ToString());
}
```
I think we need to keep one-key-one-value pairs, in order to make user parse the result of `config get *` easily.
--
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 #1047: fix: supports multiple rename-commands
Posted by GitBox <gi...@apache.org>.
git-hulk commented on code in PR #1047:
URL: https://github.com/apache/incubator-kvrocks/pull/1047#discussion_r1007805771
##########
src/config/config.cc:
##########
@@ -273,23 +273,26 @@ void Config::initFieldValidator() {
}},
{"rename-command",
[](const std::string &k, const std::string &v) -> Status {
- std::vector<std::string> args = Util::Split(v, " \t");
- if (args.size() != 2) {
- return Status(Status::NotOK, "Invalid rename-command format");
- }
- auto commands = Redis::GetCommands();
- auto cmd_iter = commands->find(Util::ToLower(args[0]));
- if (cmd_iter == commands->end()) {
- return Status(Status::NotOK, "No such command in rename-command");
- }
- if (args[1] != "\"\"") {
- auto new_command_name = Util::ToLower(args[1]);
- if (commands->find(new_command_name) != commands->end()) {
- return Status(Status::NotOK, "Target command name already exists");
+ std::vector<std::string> all_args = Util::Split(v, "\n");
Review Comment:
Okay, very sad that I made the field value as a 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.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org