You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "gloof11 (via GitHub)" <gi...@apache.org> on 2023/06/16 16:09:36 UTC
[GitHub] [incubator-kvrocks] gloof11 opened a new pull request, #1498: Warning message for invalid key: Issue #1497
gloof11 opened a new pull request, #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498
This is a fix that I was able to come up with!
It's hacky, but it works.
--
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 #1498: Warning message for invalid key in configuration file
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#issuecomment-1595020739
Thanks for your contribution!
The code need to be formatted to pass the CI, refer to https://kvrocks.apache.org/community/contributing#code-style.
--
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] infdahai commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233097516
##########
src/common/status.h:
##########
@@ -174,7 +174,7 @@ struct StringInStatusOr<T, std::enable_if_t<sizeof(T) < sizeof(std::string)>> :
StringInStatusOr(StringInStatusOr<U>&& v) : BaseType(new std::string(*std::move(v))) {} // NOLINT
template <typename U, typename std::enable_if_t<!StringInStatusOr<U>::inplace, int> = 0>
StringInStatusOr(StringInStatusOr<U>&& v) // NOLINT
- : BaseType((typename StringInStatusOr<U>::BaseType &&)(std::move(v))) {}
+ : BaseType((typename StringInStatusOr<U>::BaseType&&)(std::move(v))) {}
Review Comment:
```suggestion
: BaseType((typename StringInStatusOr<U>::BaseType &&)(std::move(v))) {}
```
Don't change this line. Just like 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] [incubator-kvrocks] infdahai commented on pull request #1498: Warning message for invalid key in configuration file
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#issuecomment-1595814280
I don't know whether it needs a test because we can't get a log message in th test. I think the code is obvious.
--
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] PragmaTwice commented on pull request #1498: Warning message for invalid key in configuration file
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1498:
URL: https://github.com/apache/kvrocks/pull/1498#issuecomment-1602361377
Hi @gloof11 , any progress or problem here?
--
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] infdahai commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1232481337
##########
src/config/config.cc:
##########
@@ -659,6 +659,12 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
tokens[input.second] = input.first.substr(ns_str_size);
}
auto iter = fields_.find(field_key);
+
+ // Check if a configuration key is a valid key
Review Comment:
Can you explain why does it work here?
We write configs to `fields_`.
https://github.com/apache/incubator-kvrocks/blob/46e7eaf428c3e4935cc73f70be3f8c9066d5cb3c/src/config/config.cc#L226-L230
--
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 pull request #1498: Warning message for invalid key in configuration file
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1498:
URL: https://github.com/apache/kvrocks/pull/1498#issuecomment-1607643884
@PragmaTwice You slashed the clang linter with the long 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] [incubator-kvrocks] torwig commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233060766
##########
src/config/config.cc:
##########
@@ -658,7 +658,13 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
field_key = input.first;
tokens[input.second] = input.first.substr(ns_str_size);
}
+
auto iter = fields_.find(field_key);
+
+ if (iter == fields_.end()) {
Review Comment:
Would you like to move the warning message inside `else` branch of `if (iter != fields_.end()) {` check which is located later?
P.S. I think you can omit a lot of `!` inside the message, just capitalize the word 'warning' as `WARNING:...`
--
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] gloof11 commented on pull request #1498: Warning message for invalid key in configuration file
Posted by "gloof11 (via GitHub)" <gi...@apache.org>.
gloof11 commented on PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#issuecomment-1595739377
Just made a commit. I read up on how maps **actually** work in Cpp. In the refactor, if the key that was passed to "iter" was not a valid key via "find()", then it would be equivalent to "past-the-end". It simply checks if both values are considered "past-the-end", and throws an error message if that's the case.
--
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] gloof11 commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "gloof11 (via GitHub)" <gi...@apache.org>.
gloof11 commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233167762
##########
src/common/status.h:
##########
@@ -174,7 +174,7 @@ struct StringInStatusOr<T, std::enable_if_t<sizeof(T) < sizeof(std::string)>> :
StringInStatusOr(StringInStatusOr<U>&& v) : BaseType(new std::string(*std::move(v))) {} // NOLINT
template <typename U, typename std::enable_if_t<!StringInStatusOr<U>::inplace, int> = 0>
StringInStatusOr(StringInStatusOr<U>&& v) // NOLINT
- : BaseType((typename StringInStatusOr<U>::BaseType &&)(std::move(v))) {}
+ : BaseType((typename StringInStatusOr<U>::BaseType&&)(std::move(v))) {}
Review Comment:
Strange, I never touched this file. When I change it to how its supposed to be formatted and run the format script, it appends the && back to BaseType.
--
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] infdahai commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233184666
##########
src/common/status.h:
##########
@@ -174,7 +174,7 @@ struct StringInStatusOr<T, std::enable_if_t<sizeof(T) < sizeof(std::string)>> :
StringInStatusOr(StringInStatusOr<U>&& v) : BaseType(new std::string(*std::move(v))) {} // NOLINT
template <typename U, typename std::enable_if_t<!StringInStatusOr<U>::inplace, int> = 0>
StringInStatusOr(StringInStatusOr<U>&& v) // NOLINT
- : BaseType((typename StringInStatusOr<U>::BaseType &&)(std::move(v))) {}
+ : BaseType((typename StringInStatusOr<U>::BaseType&&)(std::move(v))) {}
Review Comment:
`./x.py format --clang-format-path clang-format-14`
I guess you're using clang-format-16.
--
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 commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1498:
URL: https://github.com/apache/kvrocks/pull/1498#discussion_r1242406265
##########
src/config/config.cc:
##########
@@ -659,12 +659,16 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
field_key = input.first;
tokens[input.second] = input.first.substr(ns_str_size);
}
+
auto iter = fields_.find(field_key);
+
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] infdahai commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1232481337
##########
src/config/config.cc:
##########
@@ -659,6 +659,12 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
tokens[input.second] = input.first.substr(ns_str_size);
}
auto iter = fields_.find(field_key);
+
+ // Check if a configuration key is a valid key
Review Comment:
Can you explain why does it works here?
We write configs to `fields_`.
https://github.com/apache/incubator-kvrocks/blob/46e7eaf428c3e4935cc73f70be3f8c9066d5cb3c/src/config/config.cc#L226-L230
--
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] torwig commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1232486389
##########
src/config/config.cc:
##########
@@ -659,6 +659,12 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
tokens[input.second] = input.first.substr(ns_str_size);
}
auto iter = fields_.find(field_key);
+
+ // Check if a configuration key is a valid key
+ if (iter->second == 0x0) {
Review Comment:
What is the magic number `0x0`?
--
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] torwig commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233184869
##########
src/common/status.h:
##########
@@ -174,7 +174,7 @@ struct StringInStatusOr<T, std::enable_if_t<sizeof(T) < sizeof(std::string)>> :
StringInStatusOr(StringInStatusOr<U>&& v) : BaseType(new std::string(*std::move(v))) {} // NOLINT
template <typename U, typename std::enable_if_t<!StringInStatusOr<U>::inplace, int> = 0>
StringInStatusOr(StringInStatusOr<U>&& v) // NOLINT
- : BaseType((typename StringInStatusOr<U>::BaseType &&)(std::move(v))) {}
+ : BaseType((typename StringInStatusOr<U>::BaseType&&)(std::move(v))) {}
Review Comment:
@infdahai Right. Before I upgraded Fedora from 37 to 38, the command `./x.py format` did its job the right way :)
--
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] tisonkun merged pull request #1498: Warning message for invalid key in configuration file
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #1498:
URL: https://github.com/apache/kvrocks/pull/1498
--
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] torwig commented on pull request #1498: Warning message for invalid key in configuration file
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#issuecomment-1595939489
@infdahai Yes, I also think that a test, in this case, is too fanatic.
--
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 #1498: Warning message for invalid key in configuration file
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233254994
##########
src/config/config.cc:
##########
@@ -658,12 +658,16 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
field_key = input.first;
tokens[input.second] = input.first.substr(ns_str_size);
}
+
auto iter = fields_.find(field_key);
+
if (iter != fields_.end()) {
auto &field = iter->second;
field->line_number = line_number;
auto s = field->Set(input.second);
if (!s.IsOK()) return s.Prefixed(fmt::format("failed to set value of field '{}'", field_key));
+ } else {
+ std::cout << fmt::format("WARNING: '{}' is not a valid configuration key!", field_key) << std::endl;
Review Comment:
```suggestion
std::cout << fmt::format("WARNING: '{}' at line {} is not a valid configuration key.", field_key, line_number) << std::endl;
```
--
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] infdahai commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233097516
##########
src/common/status.h:
##########
@@ -174,7 +174,7 @@ struct StringInStatusOr<T, std::enable_if_t<sizeof(T) < sizeof(std::string)>> :
StringInStatusOr(StringInStatusOr<U>&& v) : BaseType(new std::string(*std::move(v))) {} // NOLINT
template <typename U, typename std::enable_if_t<!StringInStatusOr<U>::inplace, int> = 0>
StringInStatusOr(StringInStatusOr<U>&& v) // NOLINT
- : BaseType((typename StringInStatusOr<U>::BaseType &&)(std::move(v))) {}
+ : BaseType((typename StringInStatusOr<U>::BaseType&&)(std::move(v))) {}
Review Comment:
```suggestion
: BaseType((typename StringInStatusOr<U>::BaseType &&)(std::move(v))) {}
```
Don't change this 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] [incubator-kvrocks] gloof11 commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "gloof11 (via GitHub)" <gi...@apache.org>.
gloof11 commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1232920527
##########
src/config/config.cc:
##########
@@ -659,6 +659,12 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
tokens[input.second] = input.first.substr(ns_str_size);
}
auto iter = fields_.find(field_key);
+
+ // Check if a configuration key is a valid key
+ if (iter->second == 0x0) {
Review Comment:
I realized that "iter" after the find operations returns an object of all possible keys. I summarized that "iter->second" points to the name of the key, so if you were to pass a key that cannot exist, it would return zero. Therefore, if it finds a key that doesn't exist, it will return 0x0.
--
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] torwig commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233183199
##########
src/common/status.h:
##########
@@ -174,7 +174,7 @@ struct StringInStatusOr<T, std::enable_if_t<sizeof(T) < sizeof(std::string)>> :
StringInStatusOr(StringInStatusOr<U>&& v) : BaseType(new std::string(*std::move(v))) {} // NOLINT
template <typename U, typename std::enable_if_t<!StringInStatusOr<U>::inplace, int> = 0>
StringInStatusOr(StringInStatusOr<U>&& v) // NOLINT
- : BaseType((typename StringInStatusOr<U>::BaseType &&)(std::move(v))) {}
+ : BaseType((typename StringInStatusOr<U>::BaseType&&)(std::move(v))) {}
Review Comment:
@gloof11 You can undo the change manually and commit + push again.
--
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] infdahai commented on pull request #1498: Warning message for invalid key in configuration file
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#issuecomment-1594947565
Hi @gloof11 can you write a cpp unit test? (probably in https://github.com/apache/incubator-kvrocks/blob/unstable/tests/cppunit/config_test.cc).
--
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] gloof11 commented on pull request #1498: Warning message for invalid key in configuration file
Posted by "gloof11 (via GitHub)" <gi...@apache.org>.
gloof11 commented on PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#issuecomment-1595740452
Also, I'm not familiar at all with writing unit tests in C++. I'm sorry :(
--
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] torwig commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "torwig (via GitHub)" <gi...@apache.org>.
torwig commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233183002
##########
src/common/status.h:
##########
@@ -174,7 +174,7 @@ struct StringInStatusOr<T, std::enable_if_t<sizeof(T) < sizeof(std::string)>> :
StringInStatusOr(StringInStatusOr<U>&& v) : BaseType(new std::string(*std::move(v))) {} // NOLINT
template <typename U, typename std::enable_if_t<!StringInStatusOr<U>::inplace, int> = 0>
StringInStatusOr(StringInStatusOr<U>&& v) // NOLINT
- : BaseType((typename StringInStatusOr<U>::BaseType &&)(std::move(v))) {}
+ : BaseType((typename StringInStatusOr<U>::BaseType&&)(std::move(v))) {}
Review Comment:
@gloof11 The change was made by `./x.py format`. On my Fedora 38, I also have this behavior.
--
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] gloof11 commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "gloof11 (via GitHub)" <gi...@apache.org>.
gloof11 commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1233067741
##########
src/config/config.cc:
##########
@@ -658,7 +658,13 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
field_key = input.first;
tokens[input.second] = input.first.substr(ns_str_size);
}
+
auto iter = fields_.find(field_key);
+
+ if (iter == fields_.end()) {
Review Comment:
Sure can
--
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] gloof11 commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "gloof11 (via GitHub)" <gi...@apache.org>.
gloof11 commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1232920769
##########
src/config/config.cc:
##########
@@ -659,6 +659,12 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
tokens[input.second] = input.first.substr(ns_str_size);
}
auto iter = fields_.find(field_key);
+
+ // Check if a configuration key is a valid key
Review Comment:
I was wondering what the properties of "fields" was. This was a very hacky fix, so I will dig deeper to see if I can make the code more "understandable" and less "magic" lol.
--
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 #1498: Warning message for invalid key in configuration file
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#issuecomment-1595631767
It seems the code is still not formatted properly.
And I think you need give a reason why you do not just check `iter == fields_.end()`. TBH I cannot understand your code.
--
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] infdahai commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "infdahai (via GitHub)" <gi...@apache.org>.
infdahai commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1232949567
##########
src/config/config.cc:
##########
@@ -659,6 +659,12 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
tokens[input.second] = input.first.substr(ns_str_size);
}
auto iter = fields_.find(field_key);
+
+ // Check if a configuration key is a valid key
+ if (iter->second == 0x0) {
Review Comment:
we don't use the magic number, because it's undefined based on https://eel.is/c++draft/iterator.requirements#general-7.
--
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 #1498: Warning message for invalid key in configuration file
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#discussion_r1232486511
##########
src/config/config.cc:
##########
@@ -659,6 +659,12 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
tokens[input.second] = input.first.substr(ns_str_size);
}
auto iter = fields_.find(field_key);
+
+ // Check if a configuration key is a valid key
Review Comment:
Actually I wonder why not check `iter == fields_.end()` 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] PragmaTwice commented on pull request #1498: Warning message for invalid key in configuration file
Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on PR #1498:
URL: https://github.com/apache/incubator-kvrocks/pull/1498#issuecomment-1596101633
You need to revert the change of `src/common/status.h` to make the CI pass.
--
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 commented on a diff in pull request #1498: Warning message for invalid key in configuration file
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #1498:
URL: https://github.com/apache/kvrocks/pull/1498#discussion_r1242432423
##########
src/config/config.cc:
##########
@@ -659,12 +659,15 @@ Status Config::parseConfigFromPair(const std::pair<std::string, std::string> &in
field_key = input.first;
tokens[input.second] = input.first.substr(ns_str_size);
}
+
auto iter = fields_.find(field_key);
if (iter != fields_.end()) {
auto &field = iter->second;
field->line_number = line_number;
auto s = field->Set(input.second);
if (!s.IsOK()) return s.Prefixed(fmt::format("failed to set value of field '{}'", field_key));
+ } else {
+ std::cout << fmt::format("WARNING: '{}' at line {} is not a valid configuration key.", field_key, line_number) << std::endl;
Review Comment:
```suggestion
std::cout << fmt::format("WARNING: '{}' at line {} is not a valid configuration key.", field_key, line_number)
<< std::endl;
```
```suggestion
std::cout << fmt::format("WARNING: '{}' at line {} is not a valid configuration key.", field_key, line_number)
<< std::endl;
```
--
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