You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2020/03/03 23:45:01 UTC
[GitHub] [helix] mgao0 opened a new pull request #849: Add REST APIs for get,
set, update RestConfig
mgao0 opened a new pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
#848
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
Added PUT to create, POST to upate, and GET to get the RestConfig for each cluster under CONFIGS.
Tested by unit tests.
### Tests
- [ ] The following tests are written for this issue:
testCreateClusterRESTConfig, testUpdateClusterRESTConfig, testGetClusterRESTConfig
- [ ] The following is the result of the "mvn test" command on the appropriate module:
(Copy & paste the result of "mvn test")
### Commits
- [ ] My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation (Optional)
- [ ] In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Code Quality
- [ ] My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r389916123
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,125 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
Review comment:
Nit: is there a reason we don't just say createRestConfig? Cluster seems a little redundant?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388118574
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,45 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
+ if (!ZKUtil.isClusterSetup(clusterName, _zkClient)) {
+ throw new HelixException("fail to update REST config. cluster: " + clusterName + " is NOT setup.");
+ }
+
+ HelixConfigScope scope = new HelixConfigScopeBuilder(ConfigScopeProperty.REST).forCluster(clusterName).build();
+ String zkPath = scope.getZkPath();
+
+ // Create "/{clusterId}/CONFIGS/REST" if it does not exist
+ String parentPath = HelixUtil.getZkParentPath(zkPath);
Review comment:
I see "HelixUtil.getZkParentPath(zkPath)" twice. I guess you are trying to use parentPath.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r391805187
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -604,6 +605,61 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
Review comment:
If there is a change to config accessor, should we add tests in TestConfigAccessor as well?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on issue #849: Add REST APIs for get, set,
update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on issue #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#issuecomment-597309014
> Please fix/improve the code where change is possible. The fact that existing code is doing it in a certain suboptimal way doesn't justify us making the same mistake. I am fine, however, with creating a separate issue for refactoring work that will involve changes in existing logic though.
Rolling out functional service for our customers at the earliest to unblock them is our top priority. We proved the current way works fine, and the new way although is clearer in code, it bears the risk of breaking things for our customers. Therefore, I considered it sub-priority for us. Also, refactoring for one thing all together is easier for us to track and debug if anything goes wrong in the future.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r389901618
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,62 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
Review comment:
Is this necessary? We should have update function with scope arugment,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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r389074920
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
Review comment:
I see. Thank you for explanation @zhangmeng916. @pkuwm Here I'm following the format that is used by other methods in this class. But the way you suggested does provide a clearer and easier way to form the message. Later we can have a separate PR to change all the log messages in Helix to adopt the sl4j {} 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388496353
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
Review comment:
I searched for sl4j logging, but couldn't find materials for what you referred to as using{}, could you please provide an example?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #849:
Add REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388706463
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
Review comment:
I think he meant something like this:
logger.info("Initialized {} pipeline", Pipeline.Type.TASK.name());
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on issue #849: Add REST APIs for get, set,
update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on issue #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#issuecomment-598397928
This PR is ready to be merged, approved by @narendly .
Final commit message:
Title: Add REST APIs for get, set, update RestConfig
Body: Added endpoints PUT to create, POST to update, and GET to get, DELETE to delete the RestConfig for clusters in ClusterAccessor. Added corresponding methods in ConfigAccessor to be used by ClusterAccessor.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r389075331
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,107 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
Review comment:
I have updated the PR. Can you review again? Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390558842
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,62 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
Review comment:
Ok. I'll create another PR to abstract the logic.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388629384
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.updateRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to update rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @GET
Review comment:
I added the "DELETE" API, please help me review again. Thank you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390478221
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,125 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+ Command command;
+ try {
+ command = getCommand(commandStr);
+ } catch (HelixException ex) {
+ return badRequest(ex.getMessage());
+ }
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
Review comment:
Since it is a not necessary check. I removed it so it is easier for users to use.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388498987
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,45 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
+ if (!ZKUtil.isClusterSetup(clusterName, _zkClient)) {
+ throw new HelixException("fail to update REST config. cluster: " + clusterName + " is NOT setup.");
+ }
+
+ HelixConfigScope scope = new HelixConfigScopeBuilder(ConfigScopeProperty.REST).forCluster(clusterName).build();
+ String zkPath = scope.getZkPath();
+
+ // Create "/{clusterId}/CONFIGS/REST" if it does not exist
+ String parentPath = HelixUtil.getZkParentPath(zkPath);
+ if (!_zkClient.exists(HelixUtil.getZkParentPath(zkPath))) {
+ ZKUtil.createOrMerge(_zkClient, parentPath, new ZNRecord(parentPath), true, true);
+ }
+
+ if (overwrite) {
Review comment:
I see.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r391816244
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -604,6 +605,61 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
Review comment:
There are tests for rest config covering set and update in TestConfigAccessor. I just added a few more cases for delete.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388491619
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
Review comment:
I checked, all the exceptions are caught in ZKUtil.createOrMerge/Replace/Update, and only log the errors, including the write failure or other types of errors. Only one case would throw HelixException which is ZKUtil.isClusterSetup(clusterName, _zkClient) returns false, so I used the notFound response type. Please let me know if you still think it is an issue and we should fix it some other way. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388495093
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,45 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
+ if (!ZKUtil.isClusterSetup(clusterName, _zkClient)) {
+ throw new HelixException("fail to update REST config. cluster: " + clusterName + " is NOT setup.");
+ }
+
+ HelixConfigScope scope = new HelixConfigScopeBuilder(ConfigScopeProperty.REST).forCluster(clusterName).build();
+ String zkPath = scope.getZkPath();
+
+ // Create "/{clusterId}/CONFIGS/REST" if it does not exist
+ String parentPath = HelixUtil.getZkParentPath(zkPath);
+ if (!_zkClient.exists(HelixUtil.getZkParentPath(zkPath))) {
+ ZKUtil.createOrMerge(_zkClient, parentPath, new ZNRecord(parentPath), true, true);
+ }
+
+ if (overwrite) {
Review comment:
Please see the comment above, I checked the existence of parent path, not current path.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #849:
Add REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r387974320
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.updateRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to update rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @GET
Review comment:
We may also need a "DELETE" API. You can double 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390600127
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,121 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateRESTConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+ Command command;
+ try {
+ command = getCommand(commandStr);
+ } catch (HelixException ex) {
+ return badRequest(ex.getMessage());
+ }
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ switch (command) {
+ case update:
+ configAccessor.updateRESTConfig(clusterId, config);
+ break;
+ case delete: {
+ HelixConfigScope scope =
+ new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.REST)
+ .forCluster(clusterId).build();
+ configAccessor.remove(scope, config.getRecord());
+ }
+ break;
+ default:
+ return badRequest("Unsupported command " + commandStr);
+ }
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to " + command + " rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @GET
+ @Path("{clusterId}/restconfig")
+ public Response getRESTConfig(@PathParam("clusterId") String clusterId) {
+ ConfigAccessor accessor = getConfigAccessor();
+ RESTConfig config = null;
+ try {
+ config = accessor.getRESTConfig(clusterId);
+ } catch (HelixException ex) {
+ _logger.info(
+ "Failed to get rest config for cluster " + clusterId + ", cluster not found, Exception: "
+ + ex);
+ } catch (Exception ex) {
+ _logger.error("Failed to get rest config for cluster " + clusterId + " Exception: " + ex);
+ return serverError(ex);
+ }
+ if (config == null) {
+ return notFound();
+ }
+ return JSONRepresentation(config.getRecord());
+ }
+
+ @DELETE
+ @Path("{clusterId}/restconfig")
+ public Response deleteRESTConfig(@PathParam("clusterId") String clusterId) {
+ ConfigAccessor accessor = getConfigAccessor();
+ try {
+ accessor.deleteRESTConfig(clusterId);
+ } catch (HelixException ex) {
+ _logger.info("Failed to delete rest config for cluster " + clusterId
Review comment:
I was considering returns OK would be fine since what you want to delete is already not there. But on second thought, return NOT_FOUND probably is more intuitive for the users. Changing it to NOT_FOUND.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390600758
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
##########
@@ -576,6 +577,70 @@ public void testEnableWagedRebalanceForAllResources() {
}
}
+ @Test
+ public void testCreateClusterRESTConfig() throws IOException {
+ System.out.println("Start test :" + TestHelper.getTestMethodName());
+ String cluster = _clusters.iterator().next();
+ RESTConfig restConfigRest = new RESTConfig(cluster);
+ restConfigRest.set(RESTConfig.SimpleFields.CUSTOMIZED_HEALTH_URL, "http://*:00");
+ put("clusters/" + cluster + "/restconfig", null, Entity
+ .entity(OBJECT_MAPPER.writeValueAsString(restConfigRest.getRecord()),
+ MediaType.APPLICATION_JSON_TYPE), Response.Status.OK.getStatusCode());
+ RESTConfig restConfigZK = _configAccessor.getRESTConfig(cluster);
+ Assert.assertEquals(restConfigZK, restConfigRest,
+ "rest config from response: " + restConfigRest + " vs rest config actually: "
+ + restConfigZK);
+ System.out.println("End test :" + TestHelper.getTestMethodName());
+ }
+
+ @Test(dependsOnMethods = "testCreateClusterRESTConfig")
+ public void testUpdateClusterRESTConfig() throws IOException {
+ System.out.println("Start test :" + TestHelper.getTestMethodName());
+ String cluster = _clusters.iterator().next();
+ RESTConfig restConfigRest = new RESTConfig(cluster);
+ // Update an entry
+ restConfigRest.set(RESTConfig.SimpleFields.CUSTOMIZED_HEALTH_URL, "http://*:01");
+ Entity entity = Entity.entity(OBJECT_MAPPER.writeValueAsString(restConfigRest.getRecord()),
+ MediaType.APPLICATION_JSON_TYPE);
+ post("clusters/" + cluster + "/restconfig", ImmutableMap.of("command", Command.update.name()),
+ entity, Response.Status.OK.getStatusCode());
+ RESTConfig restConfigZK = _configAccessor.getRESTConfig(cluster);
+ Assert.assertEquals(restConfigZK, restConfigRest,
+ "rest config from response: " + restConfigRest + " vs rest config actually: "
+ + restConfigZK);
+
+ // Delete an entry
+ restConfigRest.set(RESTConfig.SimpleFields.CUSTOMIZED_HEALTH_URL, null);
+ entity = Entity.entity(OBJECT_MAPPER.writeValueAsString(restConfigRest.getRecord()),
+ MediaType.APPLICATION_JSON_TYPE);
+ post("clusters/" + cluster + "/restconfig", ImmutableMap.of("command", Command.delete.name()),
+ entity, Response.Status.OK.getStatusCode());
+ restConfigZK = _configAccessor.getRESTConfig(cluster);
+ Assert.assertEquals(restConfigZK, new RESTConfig(cluster),
+ "rest config from response: " + new RESTConfig(cluster) + " vs rest config actually: "
+ + restConfigZK);
+ System.out.println("End test :" + TestHelper.getTestMethodName());
Review comment:
Added one more test after the first draft. Moving it down.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly merged pull request #849: Add REST APIs for get,
set, update RestConfig
Posted by GitBox <gi...@apache.org>.
narendly merged pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r389915590
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,125 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+ Command command;
+ try {
+ command = getCommand(commandStr);
+ } catch (HelixException ex) {
+ return badRequest(ex.getMessage());
+ }
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
Review comment:
Could we double-check if the ID for the restConfig is supposed to match up with the cluster name?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on issue #849: Add REST APIs for get, set,
update RestConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on issue #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#issuecomment-597280161
Please fix/improve the code where change is possible. The fact that existing code is doing it in a certain suboptimal way doesn't justify us making the same mistake. I am fine, however, with creating a separate issue for refactoring work that will involve changes in existing logic though.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390583078
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,121 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateRESTConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+ Command command;
+ try {
+ command = getCommand(commandStr);
+ } catch (HelixException ex) {
+ return badRequest(ex.getMessage());
+ }
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ switch (command) {
+ case update:
+ configAccessor.updateRESTConfig(clusterId, config);
+ break;
+ case delete: {
+ HelixConfigScope scope =
+ new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.REST)
+ .forCluster(clusterId).build();
+ configAccessor.remove(scope, config.getRecord());
+ }
+ break;
+ default:
+ return badRequest("Unsupported command " + commandStr);
+ }
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to " + command + " rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @GET
+ @Path("{clusterId}/restconfig")
+ public Response getRESTConfig(@PathParam("clusterId") String clusterId) {
+ ConfigAccessor accessor = getConfigAccessor();
+ RESTConfig config = null;
+ try {
+ config = accessor.getRESTConfig(clusterId);
+ } catch (HelixException ex) {
+ _logger.info(
+ "Failed to get rest config for cluster " + clusterId + ", cluster not found, Exception: "
+ + ex);
+ } catch (Exception ex) {
+ _logger.error("Failed to get rest config for cluster " + clusterId + " Exception: " + ex);
+ return serverError(ex);
+ }
+ if (config == null) {
+ return notFound();
+ }
+ return JSONRepresentation(config.getRecord());
+ }
+
+ @DELETE
+ @Path("{clusterId}/restconfig")
+ public Response deleteRESTConfig(@PathParam("clusterId") String clusterId) {
+ ConfigAccessor accessor = getConfigAccessor();
+ try {
+ accessor.deleteRESTConfig(clusterId);
+ } catch (HelixException ex) {
+ _logger.info("Failed to delete rest config for cluster " + clusterId
Review comment:
Failed to delete but finally returning OK response? Would 404 be more appropriate?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390599235
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,125 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+ Command command;
+ try {
+ command = getCommand(commandStr);
+ } catch (HelixException ex) {
+ return badRequest(ex.getMessage());
+ }
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ switch (command) {
+ case update:
+ configAccessor.updateRESTConfig(clusterId, config);
+ break;
+ case delete: {
+ HelixConfigScope scope =
+ new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.REST)
+ .forCluster(clusterId).build();
+ configAccessor.remove(scope, config.getRecord());
+ }
+ break;
+ default:
+ return badRequest("Unsupported command " + commandStr);
+ }
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to " + command + " rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @GET
+ @Path("{clusterId}/restconfig")
+ public Response getClusterRESTConfig(@PathParam("clusterId") String clusterId) {
+ ConfigAccessor accessor = getConfigAccessor();
+ RESTConfig config = null;
Review comment:
I tried it, the code doesn't compile after removing "=null". I agree that initialization to null is functional redundant, but it benefits the style check and readability of the code, so I would keep it 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388497197
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,45 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
+ if (!ZKUtil.isClusterSetup(clusterName, _zkClient)) {
+ throw new HelixException("fail to update REST config. cluster: " + clusterName + " is NOT setup.");
+ }
+
+ HelixConfigScope scope = new HelixConfigScopeBuilder(ConfigScopeProperty.REST).forCluster(clusterName).build();
+ String zkPath = scope.getZkPath();
+
+ // Create "/{clusterId}/CONFIGS/REST" if it does not exist
+ String parentPath = HelixUtil.getZkParentPath(zkPath);
Review comment:
What I meant is that line 637 could be:
_zkClient.exists(parentPath)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388119405
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,45 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
+ if (!ZKUtil.isClusterSetup(clusterName, _zkClient)) {
+ throw new HelixException("fail to update REST config. cluster: " + clusterName + " is NOT setup.");
+ }
+
+ HelixConfigScope scope = new HelixConfigScopeBuilder(ConfigScopeProperty.REST).forCluster(clusterName).build();
+ String zkPath = scope.getZkPath();
+
+ // Create "/{clusterId}/CONFIGS/REST" if it does not exist
+ String parentPath = HelixUtil.getZkParentPath(zkPath);
+ if (!_zkClient.exists(HelixUtil.getZkParentPath(zkPath))) {
+ ZKUtil.createOrMerge(_zkClient, parentPath, new ZNRecord(parentPath), true, true);
+ }
+
+ if (overwrite) {
Review comment:
Actually, if it is "overwrite", there is no need to check existence. Just call createOrReplace().
If it is not "overwrite", there is no need to check existence either, just call createOrUpdate().
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390584821
##########
File path: helix-rest/src/test/java/org/apache/helix/rest/server/TestClusterAccessor.java
##########
@@ -576,6 +577,70 @@ public void testEnableWagedRebalanceForAllResources() {
}
}
+ @Test
+ public void testCreateClusterRESTConfig() throws IOException {
+ System.out.println("Start test :" + TestHelper.getTestMethodName());
+ String cluster = _clusters.iterator().next();
+ RESTConfig restConfigRest = new RESTConfig(cluster);
+ restConfigRest.set(RESTConfig.SimpleFields.CUSTOMIZED_HEALTH_URL, "http://*:00");
+ put("clusters/" + cluster + "/restconfig", null, Entity
+ .entity(OBJECT_MAPPER.writeValueAsString(restConfigRest.getRecord()),
+ MediaType.APPLICATION_JSON_TYPE), Response.Status.OK.getStatusCode());
+ RESTConfig restConfigZK = _configAccessor.getRESTConfig(cluster);
+ Assert.assertEquals(restConfigZK, restConfigRest,
+ "rest config from response: " + restConfigRest + " vs rest config actually: "
+ + restConfigZK);
+ System.out.println("End test :" + TestHelper.getTestMethodName());
+ }
+
+ @Test(dependsOnMethods = "testCreateClusterRESTConfig")
+ public void testUpdateClusterRESTConfig() throws IOException {
+ System.out.println("Start test :" + TestHelper.getTestMethodName());
+ String cluster = _clusters.iterator().next();
+ RESTConfig restConfigRest = new RESTConfig(cluster);
+ // Update an entry
+ restConfigRest.set(RESTConfig.SimpleFields.CUSTOMIZED_HEALTH_URL, "http://*:01");
+ Entity entity = Entity.entity(OBJECT_MAPPER.writeValueAsString(restConfigRest.getRecord()),
+ MediaType.APPLICATION_JSON_TYPE);
+ post("clusters/" + cluster + "/restconfig", ImmutableMap.of("command", Command.update.name()),
+ entity, Response.Status.OK.getStatusCode());
+ RESTConfig restConfigZK = _configAccessor.getRESTConfig(cluster);
+ Assert.assertEquals(restConfigZK, restConfigRest,
+ "rest config from response: " + restConfigRest + " vs rest config actually: "
+ + restConfigZK);
+
+ // Delete an entry
+ restConfigRest.set(RESTConfig.SimpleFields.CUSTOMIZED_HEALTH_URL, null);
+ entity = Entity.entity(OBJECT_MAPPER.writeValueAsString(restConfigRest.getRecord()),
+ MediaType.APPLICATION_JSON_TYPE);
+ post("clusters/" + cluster + "/restconfig", ImmutableMap.of("command", Command.delete.name()),
+ entity, Response.Status.OK.getStatusCode());
+ restConfigZK = _configAccessor.getRESTConfig(cluster);
+ Assert.assertEquals(restConfigZK, new RESTConfig(cluster),
+ "rest config from response: " + new RESTConfig(cluster) + " vs rest config actually: "
+ + restConfigZK);
+ System.out.println("End test :" + TestHelper.getTestMethodName());
Review comment:
Is the test already ended?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390030396
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,125 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+ Command command;
+ try {
+ command = getCommand(commandStr);
+ } catch (HelixException ex) {
+ return badRequest(ex.getMessage());
+ }
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
Review comment:
This check is not imposed on rest config id, this is to check the if the id of ZNRecord user uses to update the rest config matches the cluster name. I'm not sure what our users are doing right now, but this could prevent accidentally update the wrong cluster.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390546787
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,62 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
Review comment:
There are lots of common code for updateRESTConfig, updateClusterConfig and so on. I am fine with this PR. But can you have another PR to abstract the logic to a private method to reduce the redundant 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r391850065
##########
File path: helix-core/src/test/java/org/apache/helix/TestConfigAccessor.java
##########
@@ -204,4 +207,56 @@ public void testSetNonexistentParticipantConfig() throws Exception {
configAccessor.close();
System.out.println("END " + clusterName + " at " + new Date(System.currentTimeMillis()));
}
+
+ @Test
+ public void testSetRestConfig() {
+ String className = TestHelper.getTestClassName();
+ String methodName = TestHelper.getTestMethodName();
+ String clusterName = className + "_" + methodName;
+
+ ZKHelixAdmin admin = new ZKHelixAdmin(ZK_ADDR);
+ admin.addCluster(clusterName, true);
+ ConfigAccessor configAccessor = new ConfigAccessor(ZK_ADDR);
+ HelixConfigScope scope = new HelixConfigScopeBuilder(ConfigScopeProperty.REST).forCluster(clusterName).build();
+ Assert.assertNull(configAccessor.getRESTConfig(clusterName));
+
+ RESTConfig restConfig = new RESTConfig(clusterName);
+ restConfig.set(RESTConfig.SimpleFields.CUSTOMIZED_HEALTH_URL, "TEST_URL");
+ configAccessor.setRESTConfig(clusterName, restConfig);
+ Assert.assertEquals(restConfig, configAccessor.getRESTConfig(clusterName));
+ }
+
+ @Test (expectedExceptions = HelixException.class)
Review comment:
Could you mark it clearly where the exception should be expected? Because it's not often easy to tell, I prefer the style of
```
try {
//your logic
Assert.fail("Exception expected!");
catch (exception) {
// Exception expected..
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390568237
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,125 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+ Command command;
+ try {
+ command = getCommand(commandStr);
+ } catch (HelixException ex) {
+ return badRequest(ex.getMessage());
+ }
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ switch (command) {
+ case update:
+ configAccessor.updateRESTConfig(clusterId, config);
+ break;
+ case delete: {
+ HelixConfigScope scope =
+ new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.REST)
+ .forCluster(clusterId).build();
+ configAccessor.remove(scope, config.getRecord());
+ }
+ break;
+ default:
+ return badRequest("Unsupported command " + commandStr);
+ }
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to " + command + " rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @GET
+ @Path("{clusterId}/restconfig")
+ public Response getClusterRESTConfig(@PathParam("clusterId") String clusterId) {
+ ConfigAccessor accessor = getConfigAccessor();
+ RESTConfig config = null;
Review comment:
Please read https://docs.oracle.com/javase/specs/jls/se7/jls7.pdf
Object references not initialized default to null. So initializing to null is redundant.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r389050489
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,107 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
Review comment:
Can you follow the style of ClusterConfig? We have entry update/delete operation with URL param.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388120044
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
Review comment:
Could it because of write failure? I'm not sure if the accessor could throw any HelixException as well?
I know it probably lists details in the message, but the response type would be notfound in any case, which is not good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390031035
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,125 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
Review comment:
I've changed the method names.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388497235
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to create rest config, cluster " + clusterId + " new config: " + content
Review comment:
I think it's because I used the old style sheet, will update to the new style sheet. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r389907277
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,62 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
Review comment:
I've adopted the pattern for ClusterConfig here. The overwrite is to be called to distinguish set and update operations. The RESTConfig instance passed in gives us the information to be used to set/update the rest config, and the scope can be derived from information in RESTConfig in the implementation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r389916393
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,125 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+ Command command;
+ try {
+ command = getCommand(commandStr);
+ } catch (HelixException ex) {
+ return badRequest(ex.getMessage());
+ }
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ switch (command) {
+ case update:
+ configAccessor.updateRESTConfig(clusterId, config);
+ break;
+ case delete: {
+ HelixConfigScope scope =
+ new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.REST)
+ .forCluster(clusterId).build();
+ configAccessor.remove(scope, config.getRecord());
+ }
+ break;
+ default:
+ return badRequest("Unsupported command " + commandStr);
+ }
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to " + command + " rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @GET
+ @Path("{clusterId}/restconfig")
+ public Response getClusterRESTConfig(@PathParam("clusterId") String clusterId) {
+ ConfigAccessor accessor = getConfigAccessor();
+ RESTConfig config = null;
Review comment:
Initializing to null is redundant.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388500769
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
Review comment:
It's better to have a more precise exception for notFound error. Or, if we use HelixException, we should use a more generic error accordingly.
Since this is a common issue, I'm fine if you add a TODO here. We can address this later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r390031783
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,125 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ @QueryParam("command") String commandStr, String content) {
+ Command command;
+ try {
+ command = getCommand(commandStr);
+ } catch (HelixException ex) {
+ return badRequest(ex.getMessage());
+ }
+
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ switch (command) {
+ case update:
+ configAccessor.updateRESTConfig(clusterId, config);
+ break;
+ case delete: {
+ HelixConfigScope scope =
+ new HelixConfigScopeBuilder(HelixConfigScope.ConfigScopeProperty.REST)
+ .forCluster(clusterId).build();
+ configAccessor.remove(scope, config.getRecord());
+ }
+ break;
+ default:
+ return badRequest("Unsupported command " + commandStr);
+ }
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to " + command + " rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @GET
+ @Path("{clusterId}/restconfig")
+ public Response getClusterRESTConfig(@PathParam("clusterId") String clusterId) {
+ ConfigAccessor accessor = getConfigAccessor();
+ RESTConfig config = null;
Review comment:
I initialize the config to null mainly to check later if the accessor got the config successfully. If there is an Exception, we catch these exceptions, so by comparing if the config is null we know if there is error in fetching the information.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] pkuwm commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r387979116
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
Review comment:
I suggest using `{}` to make advantage of sl4j logging.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] zhangmeng916 commented on a change in pull request #849:
Add REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
zhangmeng916 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r387974587
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,89 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error(
+ "Failed to create rest config, cluster " + clusterId + " new config: " + content
Review comment:
try "code reformat" to see whether it'll look 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #849: Add
REST APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r389149290
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -511,6 +512,107 @@ public Response removeClusterStateModelDefinition(@PathParam("clusterId") String
return OK();
}
+ @PUT
+ @Path("{clusterId}/restconfig")
+ public Response createClusterRESTConfig(@PathParam("clusterId") String clusterId,
+ String content) {
+ ZNRecord record;
+ try {
+ record = toZNRecord(content);
+ } catch (IOException e) {
+ _logger.error("Failed to deserialize user's input " + content + ", Exception: " + e);
+ return badRequest("Input is not a valid ZNRecord!");
+ }
+
+ if (!record.getId().equals(clusterId)) {
+ return badRequest("ID does not match the cluster name in input!");
+ }
+
+ RESTConfig config = new RESTConfig(record);
+ ConfigAccessor configAccessor = getConfigAccessor();
+ try {
+ configAccessor.setRESTConfig(clusterId, config);
+ } catch (HelixException ex) {
+ // TODO: Could use a more generic error for HelixException
+ return notFound(ex.getMessage());
+ } catch (Exception ex) {
+ _logger.error("Failed to create rest config, cluster " + clusterId + " new config: " + content
+ + ", Exception: " + ex);
+ return serverError(ex);
+ }
+ return OK();
+ }
+
+ @POST
+ @Path("{clusterId}/restconfig")
+ public Response updateClusterRESTConfig(@PathParam("clusterId") String clusterId,
Review comment:
I think you have missunderstanding about delete the Config and delete entry of the Config.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] mgao0 commented on a change in pull request #849: Add REST
APIs for get, set, update RestConfig
Posted by GitBox <gi...@apache.org>.
mgao0 commented on a change in pull request #849: Add REST APIs for get, set, update RestConfig
URL: https://github.com/apache/helix/pull/849#discussion_r388494177
##########
File path: helix-core/src/main/java/org/apache/helix/ConfigAccessor.java
##########
@@ -605,6 +606,45 @@ public RESTConfig getRESTConfig(String clusterName) {
return new RESTConfig(record);
}
+ /**
+ * Set RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the RestConfig to be set to the cluster
+ */
+ public void setRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, true);
+ }
+
+ /**
+ * Update RestConfig of a given cluster
+ * @param clusterName the cluster id
+ * @param restConfig the new RestConfig to be set to the cluster
+ */
+ public void updateRESTConfig(String clusterName, RESTConfig restConfig) {
+ updateRESTConfig(clusterName, restConfig, false);
+ }
+
+ private void updateRESTConfig(String clusterName, RESTConfig restConfig, boolean overwrite) {
+ if (!ZKUtil.isClusterSetup(clusterName, _zkClient)) {
+ throw new HelixException("fail to update REST config. cluster: " + clusterName + " is NOT setup.");
+ }
+
+ HelixConfigScope scope = new HelixConfigScopeBuilder(ConfigScopeProperty.REST).forCluster(clusterName).build();
+ String zkPath = scope.getZkPath();
+
+ // Create "/{clusterId}/CONFIGS/REST" if it does not exist
+ String parentPath = HelixUtil.getZkParentPath(zkPath);
Review comment:
You are right. I'm trying to use the parentPath. I'm trying to resolve the problem that when we want to update the rest config in the path "/{clusterId}/CONFIGS/REST{clusterId}", the parent path "/{clusterId}/CONFIGS/REST" doesn't exist yet, so I would create the parentPath first.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org