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/10 04:34:33 UTC
[GitHub] [helix] alirezazamani opened a new pull request #882: Change the
REST call for delete CloudConfig
alirezazamani opened a new pull request #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #881
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
In this PR, the necessary changes has been made to the REST API call for cloud config deletion.
Previously we were using POST (command.delete) to delete the cloud config. With this new change, we can use DELETE request to delete the cloud config.
The tests has been changed accordingly.
### Tests
- [x] The following is the result of the "mvn test" command on the appropriate module:
helix-core:
[INFO] Results:
[INFO]
[INFO] Tests run: 906, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 57:50 min
[INFO] Finished at: 2020-03-09T21:16:59-07:00
[INFO] ------------------------------------------------------------------------
helix-rest:
[INFO] Tests run: 99, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 26.976 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 99, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 32.417 s
[INFO] Finished at: 2020-03-09T21:32:04-07:00
[INFO] ------------------------------------------------------------------------
### Commits
- [x] My commits all reference appropriate Apache Helix GitHub issues in their subject lines, and I have squashed multiple commits if they address the same issue. 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"
### Code Quality
- [x] My diff has been formatted using helix-style.xml
----------------------------------------------------------------
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] alirezazamani commented on a change in pull request #882:
Change the REST call for delete CloudConfig
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#discussion_r391255001
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -617,10 +630,6 @@ public Response updateCloudConfig(@PathParam("clusterId") String clusterId,
}
try {
switch (command) {
- case delete: {
- admin.removeCloudConfig(clusterId);
- }
- break;
case update: {
try {
CloudConfig cloudConfig = new CloudConfig.Builder(record).build();
Review comment:
I will do new PR for update/delete the entries.
----------------------------------------------------------------
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] alirezazamani commented on issue #882: Change the REST call
for delete CloudConfig
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on issue #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#issuecomment-598851071
This PR is ready to be merged, approved by @dasahcc.
Final commit message:
Change the delete CloudConfig
Use DELETE instead of POST for deletion of the CloudConfig in REST.
----------------------------------------------------------------
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 merged pull request #882: Change the REST call for
delete CloudConfig
Posted by GitBox <gi...@apache.org>.
dasahcc merged pull request #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882
----------------------------------------------------------------
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] alirezazamani commented on a change in pull request #882:
Change the REST call for delete CloudConfig
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#discussion_r391255431
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -584,6 +584,19 @@ public Response getCloudConfig(@PathParam("clusterId") String clusterId) {
return notFound();
}
+ @DELETE
+ @Path("{clusterId}/cloudconfig")
+ public Response deleteCloudConfig(@PathParam("clusterId") String clusterId) {
+ HelixZkClient zkClient = getHelixZkClient();
+ if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound();
+ }
+
+ HelixAdmin admin = getHelixAdmin();
+ admin.removeCloudConfig(clusterId);
+ return OK();
Review comment:
I also changed the tests accordingly.
----------------------------------------------------------------
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 #882: Change
the REST call for delete CloudConfig
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#discussion_r390514649
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -617,10 +630,6 @@ public Response updateCloudConfig(@PathParam("clusterId") String clusterId,
}
try {
switch (command) {
- case delete: {
- admin.removeCloudConfig(clusterId);
- }
- break;
case update: {
try {
CloudConfig cloudConfig = new CloudConfig.Builder(record).build();
Review comment:
Can you have another PR like Molly's work? The update/delete for a cloudConfig is working on update/delete entry of the CloudConfig.
----------------------------------------------------------------
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] alirezazamani commented on a change in pull request #882:
Change the REST call for delete CloudConfig
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#discussion_r392390941
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -584,6 +584,15 @@ public Response getCloudConfig(@PathParam("clusterId") String clusterId) {
return notFound();
}
+ @DELETE
+ @Path("{clusterId}/cloudconfig")
+ public Response deleteCloudConfig(@PathParam("clusterId") String clusterId) {
+ HelixZkClient zkClient = getHelixZkClient();
Review comment:
Done.
----------------------------------------------------------------
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 issue #882: Change the REST call for
delete CloudConfig
Posted by GitBox <gi...@apache.org>.
dasahcc commented on issue #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#issuecomment-598308793
But we need update/delete operation for per entry in REST 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] dasahcc commented on a change in pull request #882: Change
the REST call for delete CloudConfig
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#discussion_r392381098
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -584,6 +584,15 @@ public Response getCloudConfig(@PathParam("clusterId") String clusterId) {
return notFound();
}
+ @DELETE
+ @Path("{clusterId}/cloudconfig")
+ public Response deleteCloudConfig(@PathParam("clusterId") String clusterId) {
+ HelixZkClient zkClient = getHelixZkClient();
Review comment:
This is not necessary.
----------------------------------------------------------------
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] alirezazamani commented on a change in pull request #882:
Change the REST call for delete CloudConfig
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#discussion_r391254789
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -584,6 +584,19 @@ public Response getCloudConfig(@PathParam("clusterId") String clusterId) {
return notFound();
}
+ @DELETE
+ @Path("{clusterId}/cloudconfig")
+ public Response deleteCloudConfig(@PathParam("clusterId") String clusterId) {
+ HelixZkClient zkClient = getHelixZkClient();
+ if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound();
+ }
+
+ HelixAdmin admin = getHelixAdmin();
+ admin.removeCloudConfig(clusterId);
+ return OK();
Review comment:
Done.
----------------------------------------------------------------
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 issue #882: Change the REST call for
delete CloudConfig
Posted by GitBox <gi...@apache.org>.
dasahcc commented on issue #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#issuecomment-598308019
Wait... Sorry for the confusing here. You have delete REST config in HelixAdmin. Then you dont need to move the whole logic to config accessor. In the ClusterAccesor, when you delete the REST config, what you need to do is call HelixAdmin's delete REST 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] dasahcc commented on a change in pull request #882: Change
the REST call for delete CloudConfig
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #882: Change the REST call for delete CloudConfig
URL: https://github.com/apache/helix/pull/882#discussion_r390514934
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ClusterAccessor.java
##########
@@ -584,6 +584,19 @@ public Response getCloudConfig(@PathParam("clusterId") String clusterId) {
return notFound();
}
+ @DELETE
+ @Path("{clusterId}/cloudconfig")
+ public Response deleteCloudConfig(@PathParam("clusterId") String clusterId) {
+ HelixZkClient zkClient = getHelixZkClient();
+ if (!ZKUtil.isClusterSetup(clusterId, zkClient)) {
+ return notFound();
+ }
+
+ HelixAdmin admin = getHelixAdmin();
+ admin.removeCloudConfig(clusterId);
+ return OK();
Review comment:
Move the logic to configAccessor.
----------------------------------------------------------------
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