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 2021/11/30 19:54:33 UTC
[GitHub] [helix] xyuanlu opened a new pull request #1917: Add rest API for take/free instance
xyuanlu opened a new pull request #1917:
URL: https://github.com/apache/helix/pull/1917
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
#1896
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
This change adds take/free instance rest API endpoint and JSON parser.
### Tests
- [X] The following tests are written for this issue:
Test will be added when take/freeInstance is implemented in clusterMaintenanceService.java
- The following is the result of the "mvn test" command on the appropriate module:
```
[INFO] Tests run: 179, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 287.922 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 179, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco-maven-plugin:0.8.6:report (generate-code-coverage-report) @ helix-rest ---
[INFO] Loading execution data file /Users/xialu/Documents/WorkSpace/helix/helix-rest/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Restful Interface' with 85 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 04:52 min
[INFO] Finished at: 2021-11-30T11:26:48-08:00
[INFO] ------------------------------------------------------------------------
```
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### 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"
### 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.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] junkaixue commented on a change in pull request #1917: Add rest API for take/free instance
Posted by GitBox <gi...@apache.org>.
junkaixue commented on a change in pull request #1917:
URL: https://github.com/apache/helix/pull/1917#discussion_r760513774
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
##########
@@ -423,6 +422,23 @@ private StoppableCheck performCustomInstanceCheck(String clusterId, String insta
return result;
}
+ public static List<String> getListFromJsonPayload(JsonNode jsonContent) throws IOException {
+ if (jsonContent == null) {
+ return Collections.emptyList();
+ }
+ return OBJECT_MAPPER.convertValue(jsonContent, List.class);
+ }
+
+ public static Map<String, String> getMapFromJsonPayload(JsonNode jsonContent) throws IOException {
+ Map<String, String> result = new HashMap<>();
+ if (jsonContent == null) {
+ return result;
+ }
+ result = OBJECT_MAPPER.convertValue(jsonContent, new TypeReference<Map<String, String>>() {
Review comment:
Would this throw exception?
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PerInstanceAccessor.java
##########
@@ -185,6 +200,137 @@ public Response isInstanceStoppable(
return OK(OBJECT_MAPPER.writeValueAsString(stoppableCheck));
}
+ /**
+ * Performs health checks, user designed operation check and execution for take an instance.
+ *
+ * @param jsonContent json payload
+ * @param clusterId cluster id
+ * @param instanceName Instance name to be checked
+ * @return json response representing if queried instance is stoppable
+ * @throws IOException if there is any IO/network error
+ */
+ @ResponseMetered(name = HttpConstants.WRITE_REQUEST)
+ @Timed(name = HttpConstants.WRITE_REQUEST)
+ @POST
+ @Path("takeInstance")
+ @Consumes(MediaType.APPLICATION_JSON)
+ public Response takeSingleInstance(
+ String jsonContent,
+ @PathParam("clusterId") String clusterId,
+ @PathParam("instanceName") String instanceName){
+
+ try {
+ JsonNode node = null;
+ if (jsonContent.length() != 0) {
+ node = OBJECT_MAPPER.readTree(jsonContent);
+ }
+ if (node == null) {
+ return badRequest("Invalid input for content : " + jsonContent);
+ }
+ MaintenanceOpInputFields inputFields = readMaintenanceInputFromJson(node);
+
+ MaintenanceManagementService maintenanceManagementService =
+ new MaintenanceManagementService((ZKHelixDataAccessor) getDataAccssor(clusterId),
+ getConfigAccessor(), inputFields.skipZKRead, inputFields.continueOnFailures,
+ getNamespace());
+
+ return JSONRepresentation(maintenanceManagementService
+ .takeInstance(clusterId, instanceName, inputFields.healthChecks,
+ inputFields.healthCheckConfig,
+ inputFields.operations,
+ inputFields.operationConfig, true));
+ } catch (Exception e) {
+ LOG.error("Failed to takeInstances:", e);
+ return badRequest("Failed to takeInstances: " + e.getMessage());
+ }
+ }
+
+ /**
+ * Performs health checks, user designed operation check and execution for free an instance.
+ *
+ * @param jsonContent json payload
+ * @param clusterId cluster id
+ * @param instanceName Instance name to be checked
+ * @return json response representing if queried instance is stoppable
+ * @throws IOException if there is any IO/network error
+ */
+ @ResponseMetered(name = HttpConstants.WRITE_REQUEST)
+ @Timed(name = HttpConstants.WRITE_REQUEST)
+ @POST
+ @Path("freeInstance")
+ @Consumes(MediaType.APPLICATION_JSON)
+ public Response freeSingleInstance(
+ String jsonContent,
+ @PathParam("clusterId") String clusterId,
+ @PathParam("instanceName") String instanceName){
+
+ try {
+ JsonNode node = null;
+ if (jsonContent.length() != 0) {
+ node = OBJECT_MAPPER.readTree(jsonContent);
+ }
+ if (node == null) {
+ return badRequest("Invalid input for content : " + jsonContent);
+ }
+ MaintenanceOpInputFields inputFields = readMaintenanceInputFromJson(node);
Review comment:
This is dup code with previous function. Let's wrap it into readMaintenanceInputFromJson function?
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/PerInstanceAccessor.java
##########
@@ -185,6 +200,137 @@ public Response isInstanceStoppable(
return OK(OBJECT_MAPPER.writeValueAsString(stoppableCheck));
}
+ /**
+ * Performs health checks, user designed operation check and execution for take an instance.
+ *
+ * @param jsonContent json payload
+ * @param clusterId cluster id
+ * @param instanceName Instance name to be checked
+ * @return json response representing if queried instance is stoppable
+ * @throws IOException if there is any IO/network error
+ */
+ @ResponseMetered(name = HttpConstants.WRITE_REQUEST)
+ @Timed(name = HttpConstants.WRITE_REQUEST)
+ @POST
+ @Path("takeInstance")
+ @Consumes(MediaType.APPLICATION_JSON)
+ public Response takeSingleInstance(
+ String jsonContent,
+ @PathParam("clusterId") String clusterId,
+ @PathParam("instanceName") String instanceName){
+
+ try {
+ JsonNode node = null;
+ if (jsonContent.length() != 0) {
+ node = OBJECT_MAPPER.readTree(jsonContent);
+ }
+ if (node == null) {
+ return badRequest("Invalid input for content : " + jsonContent);
+ }
+ MaintenanceOpInputFields inputFields = readMaintenanceInputFromJson(node);
+
+ MaintenanceManagementService maintenanceManagementService =
+ new MaintenanceManagementService((ZKHelixDataAccessor) getDataAccssor(clusterId),
+ getConfigAccessor(), inputFields.skipZKRead, inputFields.continueOnFailures,
+ getNamespace());
+
+ return JSONRepresentation(maintenanceManagementService
+ .takeInstance(clusterId, instanceName, inputFields.healthChecks,
+ inputFields.healthCheckConfig,
+ inputFields.operations,
+ inputFields.operationConfig, true));
+ } catch (Exception e) {
+ LOG.error("Failed to takeInstances:", e);
+ return badRequest("Failed to takeInstances: " + e.getMessage());
+ }
+ }
+
+ /**
+ * Performs health checks, user designed operation check and execution for free an instance.
+ *
+ * @param jsonContent json payload
+ * @param clusterId cluster id
+ * @param instanceName Instance name to be checked
+ * @return json response representing if queried instance is stoppable
+ * @throws IOException if there is any IO/network error
+ */
+ @ResponseMetered(name = HttpConstants.WRITE_REQUEST)
+ @Timed(name = HttpConstants.WRITE_REQUEST)
+ @POST
+ @Path("freeInstance")
+ @Consumes(MediaType.APPLICATION_JSON)
+ public Response freeSingleInstance(
+ String jsonContent,
+ @PathParam("clusterId") String clusterId,
+ @PathParam("instanceName") String instanceName){
+
+ try {
+ JsonNode node = null;
+ if (jsonContent.length() != 0) {
+ node = OBJECT_MAPPER.readTree(jsonContent);
+ }
+ if (node == null) {
+ return badRequest("Invalid input for content : " + jsonContent);
+ }
+ MaintenanceOpInputFields inputFields = readMaintenanceInputFromJson(node);
+ if (inputFields.healthChecks.size() != 0) {
+ LOG.warn("freeSingleInstance won't perform user passed health check.");
+ }
+
+ MaintenanceManagementService maintenanceManagementService =
+ new MaintenanceManagementService((ZKHelixDataAccessor) getDataAccssor(clusterId),
+ getConfigAccessor(), inputFields.skipZKRead, inputFields.continueOnFailures,
+ getNamespace());
+
+ return JSONRepresentation(maintenanceManagementService
+ .freeInstance(clusterId, instanceName, inputFields.healthChecks,
+ inputFields.healthCheckConfig,
+ inputFields.operations,
+ inputFields.operationConfig, true));
+ } catch (Exception e) {
+ LOG.error("Failed to takeInstances:", e);
+ return badRequest("Failed to takeInstances: " + e.getMessage());
+ }
+ }
+
+ private MaintenanceOpInputFields readMaintenanceInputFromJson(JsonNode node) throws IOException {
+ MaintenanceOpInputFields inputFields = new MaintenanceOpInputFields();
+ String continueOnFailuresName = PerInstanceProperties.continueOnFailures.name();
+ String skipZKReadName = PerInstanceProperties.skipZKRead.name();
+
+ inputFields.continueOnFailures =
+ inputFields.healthCheckConfig != null && inputFields.healthCheckConfig
+ .containsKey(continueOnFailuresName) && Boolean
+ .parseBoolean(inputFields.healthCheckConfig.get(continueOnFailuresName));
+ inputFields.skipZKRead = inputFields.healthCheckConfig != null && inputFields.healthCheckConfig
+ .containsKey(skipZKReadName) && Boolean
+ .parseBoolean(inputFields.healthCheckConfig.get(skipZKReadName));
+
+ inputFields.healthChecks = MaintenanceManagementService
+ .getListFromJsonPayload(node.get(PerInstanceProperties.health_check_list.name()));
+ inputFields.healthCheckConfig = MaintenanceManagementService
+ .getMapFromJsonPayload(node.get(PerInstanceProperties.health_check_config.name()));
+ if (inputFields.healthCheckConfig != null || !inputFields.healthChecks.isEmpty()) {
+ inputFields.healthCheckConfig.remove(continueOnFailuresName);
+ inputFields.healthCheckConfig.remove(skipZKReadName);
+ }
+
+ inputFields.operations = MaintenanceManagementService
+ .getListFromJsonPayload(node.get(PerInstanceProperties.operation_list.name()));
+ inputFields.operationConfig = MaintenanceManagementService
+ .getMapFromJsonPayload(node.get(PerInstanceProperties.operation_config.name()));
+
+ LOG.debug("healthChecks: " + (inputFields.healthChecks == null ? "NULL"
Review comment:
Can we just use inputFields.toString()?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a change in pull request #1917: Add rest API for take/free instance
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1917:
URL: https://github.com/apache/helix/pull/1917#discussion_r760504682
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
##########
@@ -412,17 +411,36 @@ private StoppableCheck performCustomInstanceCheck(String clusterId, String insta
return instanceStoppableChecks;
}
- private Map<String, String> getCustomPayLoads(String jsonContent) throws IOException {
+ public static Map<String, String> getMapFromJsonPayload(String jsonContent) throws IOException {
Map<String, String> result = new HashMap<>();
if (jsonContent == null) {
return result;
}
+
JsonNode jsonNode = OBJECT_MAPPER.readTree(jsonContent);
// parsing the inputs as string key value pairs
jsonNode.fields().forEachRemaining(kv -> result.put(kv.getKey(), kv.getValue().asText()));
return result;
}
+ public static List<String> getListFromJsonPayload(JsonNode jsonContent) throws IOException {
+ if (jsonContent == null) {
+ return new ArrayList<>();
Review comment:
TFTR. Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
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 #1917: Add rest API for take/free instance
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1917:
URL: https://github.com/apache/helix/pull/1917#discussion_r760537773
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/api/OperationAbstractClass.java
##########
@@ -26,7 +26,7 @@
import org.apache.helix.rest.common.datamodel.RestSnapShot;
-interface OperationAbstractClass {
+public interface OperationAbstractClass {
Review comment:
An interface with "Abstract" in its name seems like a code smell?
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
##########
@@ -423,6 +422,23 @@ private StoppableCheck performCustomInstanceCheck(String clusterId, String insta
return result;
}
+ public static List<String> getListFromJsonPayload(JsonNode jsonContent) throws IOException {
+ if (jsonContent == null) {
+ return Collections.emptyList();
+ }
+ return OBJECT_MAPPER.convertValue(jsonContent, List.class);
+ }
+
+ public static Map<String, String> getMapFromJsonPayload(JsonNode jsonContent) throws IOException {
+ Map<String, String> result = new HashMap<>();
+ if (jsonContent == null) {
+ return result;
+ }
Review comment:
You could simplify this logic with the use of a ternary operator, which would let you save the work of instantiating an empty HashMap instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
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 #1917: Add rest API for take/free instance
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1917:
URL: https://github.com/apache/helix/pull/1917#discussion_r760499544
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
##########
@@ -412,17 +411,36 @@ private StoppableCheck performCustomInstanceCheck(String clusterId, String insta
return instanceStoppableChecks;
}
- private Map<String, String> getCustomPayLoads(String jsonContent) throws IOException {
+ public static Map<String, String> getMapFromJsonPayload(String jsonContent) throws IOException {
Map<String, String> result = new HashMap<>();
if (jsonContent == null) {
return result;
}
+
JsonNode jsonNode = OBJECT_MAPPER.readTree(jsonContent);
// parsing the inputs as string key value pairs
jsonNode.fields().forEachRemaining(kv -> result.put(kv.getKey(), kv.getValue().asText()));
return result;
}
+ public static List<String> getListFromJsonPayload(JsonNode jsonContent) throws IOException {
+ if (jsonContent == null) {
+ return new ArrayList<>();
Review comment:
`Collections.emptyList()` might be better here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on pull request #1917: Add rest API for take/free instance
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on pull request #1917:
URL: https://github.com/apache/helix/pull/1917#issuecomment-984984383
This PR is ready to be checked in. Approved by @junkaixue //Thanks!
Commit message:
Add rest API for take/free instance
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] xyuanlu commented on a change in pull request #1917: Add rest API for take/free instance
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1917:
URL: https://github.com/apache/helix/pull/1917#discussion_r760640784
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/clusterMaintenanceService/MaintenanceManagementService.java
##########
@@ -423,6 +422,23 @@ private StoppableCheck performCustomInstanceCheck(String clusterId, String insta
return result;
}
+ public static List<String> getListFromJsonPayload(JsonNode jsonContent) throws IOException {
+ if (jsonContent == null) {
+ return Collections.emptyList();
+ }
+ return OBJECT_MAPPER.convertValue(jsonContent, List.class);
+ }
+
+ public static Map<String, String> getMapFromJsonPayload(JsonNode jsonContent) throws IOException {
+ Map<String, String> result = new HashMap<>();
+ if (jsonContent == null) {
+ return result;
+ }
Review comment:
TFTR. Good suggestion. Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] junkaixue merged pull request #1917: Add rest API for take/free instance
Posted by GitBox <gi...@apache.org>.
junkaixue merged pull request #1917:
URL: https://github.com/apache/helix/pull/1917
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org