You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "git-hulk (via GitHub)" <gi...@apache.org> on 2024/11/10 12:43:50 UTC

[PR] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

git-hulk opened a new pull request, #2654:
URL: https://github.com/apache/kvrocks/pull/2654

   This closes #2583.


-- 
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] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654#discussion_r1835689156


##########
src/types/redis_json.h:
##########
@@ -82,6 +82,7 @@ class Json : public Database {
 
   rocksdb::Status Resp(engine::Context &ctx, const std::string &user_key, const std::string &path,
                        std::vector<std::string> *results, RESP resp);
+  static rocksdb::Status FromString(const std::string &value, JsonValue *result);

Review Comment:
   If you really need it, please rename it to `FromRawString`. `FromString` is a weird name 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

Re: [PR] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk commented on code in PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654#discussion_r1835726373


##########
src/types/redis_json.h:
##########
@@ -82,6 +82,7 @@ class Json : public Database {
 
   rocksdb::Status Resp(engine::Context &ctx, const std::string &user_key, const std::string &path,
                        std::vector<std::string> *results, RESP resp);
+  static rocksdb::Status FromString(const std::string &value, JsonValue *result);

Review Comment:
   Yes, `FromRawString` would be better.



-- 
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] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654#issuecomment-2466798412

   ## [![Quality Gate Failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-failed-20px.png 'Quality Gate Failed')](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2654) **Quality Gate failed**  
   Failed conditions  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/failed-16px.png '') [C Reliability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2654) (required ≥ A)  
     
   [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2654)
   
   ##   
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/light_bulb-16px.png '') Catch issues before they fail your Quality Gate with our IDE extension ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/sonarlint-16px.png '') [SonarLint](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
   
   


-- 
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] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654#discussion_r1835689156


##########
src/types/redis_json.h:
##########
@@ -82,6 +82,7 @@ class Json : public Database {
 
   rocksdb::Status Resp(engine::Context &ctx, const std::string &user_key, const std::string &path,
                        std::vector<std::string> *results, RESP resp);
+  static rocksdb::Status FromString(const std::string &value, JsonValue *result);

Review Comment:
   If it's really needed, please rename it to `FromRawString`. `FromString` is a weird name 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

Re: [PR] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654#issuecomment-2469343022

   ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2654) **Quality Gate passed**  
   Issues  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [1 New issue](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2654&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/accepted-16px.png '') [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_kvrocks&pullRequest=2654&issueStatuses=ACCEPTED)
   
   Measures  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_kvrocks&pullRequest=2654&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [60.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2654&metric=new_coverage&view=list)  
   ![](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/passed-16px.png '') [2.5% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_kvrocks&pullRequest=2654&metric=new_duplicated_lines_density&view=list)  
     
   [See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_kvrocks&pullRequest=2654)
   
   


-- 
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] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654#discussion_r1835688877


##########
src/types/redis_json.cc:
##########
@@ -674,4 +674,12 @@ rocksdb::Status Json::Resp(engine::Context &ctx, const std::string &user_key, co
   return rocksdb::Status::OK();
 }
 
+rocksdb::Status Json::FromString(const std::string &value, JsonValue *result) {
+  Slice rest = value;
+  JsonMetadata metadata;
+  auto s = ParseMetadata({kRedisJson}, &rest, &metadata);
+  if (!s.ok()) return s;
+  return parse(metadata, rest, result);
+}
+

Review Comment:
   It seems useless. `Json::read` has same 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

Re: [PR] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654#discussion_r1835694300


##########
src/types/redis_json.cc:
##########
@@ -674,4 +674,12 @@ rocksdb::Status Json::Resp(engine::Context &ctx, const std::string &user_key, co
   return rocksdb::Status::OK();
 }
 
+rocksdb::Status Json::FromString(const std::string &value, JsonValue *result) {
+  Slice rest = value;
+  JsonMetadata metadata;
+  auto s = ParseMetadata({kRedisJson}, &rest, &metadata);
+  if (!s.ok()) return s;
+  return parse(metadata, rest, result);
+}
+

Review Comment:
   Ahhh my fault. It's not same.



-- 
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] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "git-hulk (via GitHub)" <gi...@apache.org>.
git-hulk merged PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654


-- 
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] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654#discussion_r1835727187


##########
src/types/redis_json.cc:
##########
@@ -674,4 +674,12 @@ rocksdb::Status Json::Resp(engine::Context &ctx, const std::string &user_key, co
   return rocksdb::Status::OK();
 }
 
+rocksdb::Status Json::FromRawString(const std::string &value, JsonValue *result) {

Review Comment:
   ```suggestion
   rocksdb::Status Json::FromRawString(std::string_view value, JsonValue *result) {
   ```



-- 
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] feat(cluster): add support of the JSON type in cluster migration [kvrocks]

Posted by "PragmaTwice (via GitHub)" <gi...@apache.org>.
PragmaTwice commented on code in PR #2654:
URL: https://github.com/apache/kvrocks/pull/2654#discussion_r1835689156


##########
src/types/redis_json.h:
##########
@@ -82,6 +82,7 @@ class Json : public Database {
 
   rocksdb::Status Resp(engine::Context &ctx, const std::string &user_key, const std::string &path,
                        std::vector<std::string> *results, RESP resp);
+  static rocksdb::Status FromString(const std::string &value, JsonValue *result);

Review Comment:
   If it's really needed, we can rename it to `FromRawString`. `FromString` is a weird name 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