You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "jonathanc-n (via GitHub)" <gi...@apache.org> on 2024/09/14 21:19:42 UTC
[PR] fix: Added new parsing check [kvrocks]
jonathanc-n opened a new pull request, #2538:
URL: https://github.com/apache/kvrocks/pull/2538
references #2401
Added non confusing message when multiple nodes are added without a newline separator
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2351722050
Test case is added + the node checking is more flexible
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2360610972
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2538) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2538&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2538&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2538&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [61.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2538&metric=new_coverage&view=list)
 [2.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2538&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2538)
--
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
Re: [PR] fix: add more check in cluster node parsing [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2365069805
@jonathanc-n No worry, thanks for your great contribution.
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2365048565
@git-hulk Alright, should be fine now, sorry about that.
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2352255761
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2538) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2538&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2538&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2538&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [61.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2538&metric=new_coverage&view=list)
 [2.1% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2538&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2538)
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2359330937
@git-hulk I simplified as much as I thought I could for the issue to not persist. I believe it should be fine.
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2365003051
@git-hulk @PragmaTwice Pretty sure this should be good, it passed the workflow. Unless you see some changes that should be made.
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2358354444
@jonathanc-n This would be better to check if the field is node id in https://github.com/apache/kvrocks/blob/2a0c57ac7fc4499a210bc452884025a0010cb470/src/cluster/cluster.cc#L778?
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2365166321
## [](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2538) **Quality Gate passed**
Issues
 [3 New issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2538&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2538&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2538&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [61.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2538&metric=new_coverage&view=list)
 [2.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2538&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2538)
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2352332814
@jonathanc-n #2401 is caused by missing a newline between nodes, and omitting the newline at the end should be allowed.
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2354449629
@jonathanc-n You can enhance this inside [parseClusterNodes](https://github.com/apache/kvrocks/blob/2a0c57ac7fc4499a210bc452884025a0010cb470/src/cluster/cluster.cc#L712)
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#discussion_r1769486574
##########
src/cluster/cluster.cc:
##########
@@ -776,6 +783,13 @@ Status Cluster::parseClusterNodes(const std::string &nodes_str, ClusterNodes *no
for (unsigned i = 5; i < fields.size(); i++) {
std::vector<std::string> ranges = util::Split(fields[i], "-");
if (ranges.size() == 1) {
+ std::regex node_id_regex(R"(\b[a-fA-F0-9]{40}\b)");
Review Comment:
It should be good to only check if `fields[i]` is a node id?
##########
src/cluster/cluster.cc:
##########
@@ -716,6 +716,13 @@ Status Cluster::parseClusterNodes(const std::string &nodes_str, ClusterNodes *no
return {Status::ClusterInvalidInfo, errInvalidClusterNodeInfo};
}
+ for (const auto &entry : nodes_info) {
+ if (entry.find(' ') == std::string::npos) {
+ return {Status::ClusterInvalidInfo,
+ "Invalid nodes definition: Each node entry must contain fields separated by spaces."};
+ }
+ }
+
Review Comment:
```suggestion
```
It has been checked at Line 730.
--
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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2356400349
@PragmaTwice @git-hulk This should be good I believe, I moved the changes to cluster.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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2353497897
@git-hulk Alright, it should be fully working 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
Re: [PR] fix: Added new parsing check [kvrocks]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on PR #2538:
URL: https://github.com/apache/kvrocks/pull/2538#issuecomment-2352031847
@PragmaTwice Are you able to see what exactly is causing the fail on the error, I don't seem to be able to see the reason for it.
--
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