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/06/08 23:21:36 UTC
[GitHub] [helix] xyuanlu opened a new pull request #1787: Support currentState format for partitionAssignment
xyuanlu opened a new pull request #1787:
URL: https://github.com/apache/helix/pull/1787
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
#1746
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
Rest API `partitionAssignment`returns optimal replica assignments of a current cluster or given future potential change. This change adds an option for user to be able to choose return format.
### Tests
- [X] The following tests are written for this issue:
testComputePartitionAssignment
- The following is the result of the "mvn test" command on the appropriate module:
```
[INFO] Results:
[INFO]
[INFO] Tests run: 178, 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 78 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:31 min
[INFO] Finished at: 2021-06-08T16:14:59-07:00
```
### 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.
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] jiajunwang commented on a change in pull request #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1787:
URL: https://github.com/apache/helix/pull/1787#discussion_r650183288
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -104,11 +107,18 @@ public AssignmentResult() {
Map<String, String> swapInstances;
}
+ private enum AssignmentFormat{
+ IdealStateFormat,
+ CurrentStateFormat
Review comment:
The convention of enum naming is uppercase with "_". So it should be IDEAL_STATE_FORMAT.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -104,11 +107,18 @@ public AssignmentResult() {
Map<String, String> swapInstances;
}
+ private enum AssignmentFormat{
+ IdealStateFormat,
+ CurrentStateFormat
Review comment:
I vote to ensure all enums in the project keep the same style. I know rest has some legacy namings that not necessarily follow the rule. But we should not add more.
Regarding which name is better, you can add a string field for the Enum object if really needed. But they make not much difference from my perspective. It is up to 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
---------------------------------------------------------------------
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 #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1787:
URL: https://github.com/apache/helix/pull/1787#discussion_r650244912
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -104,11 +107,18 @@ public AssignmentResult() {
Map<String, String> swapInstances;
}
+ private enum AssignmentFormat{
+ IdealStateFormat,
+ CurrentStateFormat
Review comment:
TFTR. Normally it is advised to have enum with this convention. However, when we want to access enum name, isn't this format better than having enum with value? (Or at lease keep all enums in REST in the same format? Like how ClusterAccessor.java and other REST resources do it.)
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -104,11 +107,18 @@ public AssignmentResult() {
Map<String, String> swapInstances;
}
+ private enum AssignmentFormat{
+ IdealStateFormat,
+ CurrentStateFormat
Review comment:
We had an offline discussion and decide to keep this way for the following reason:
1. Change enum name to uppercase like `IDEAL_STATE_FORMAT` and keep the same format in input JSON. This is not optimal because it will impact user readability in input.
2. Because we are doing auto converting from JSON input, adding a string field for the Enum will require more code change like comparing the string many times and have dedicated sanity check 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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang merged pull request #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1787:
URL: https://github.com/apache/helix/pull/1787
--
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
---------------------------------------------------------------------
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 #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on pull request #1787:
URL: https://github.com/apache/helix/pull/1787#issuecomment-859970911
This PR is ready to be merged. Approved by @jiajunwang
Final commit message:
Support currentState format for partitionAssignment.
--
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
---------------------------------------------------------------------
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 #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1787:
URL: https://github.com/apache/helix/pull/1787#discussion_r648531571
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -104,11 +107,16 @@ public AssignmentResult() {
Map<String, String> swapInstances;
}
+ private static final String IDEAL_STATE_FORMAT = "IdealStateFormat";
+ private static final String CURRENT_STATE_FORMAT = "CurrentStateFormat";
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.
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] jiajunwang commented on a change in pull request #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1787:
URL: https://github.com/apache/helix/pull/1787#discussion_r648544235
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -208,6 +220,13 @@ private ClusterState readClusterStateAndValidateInput(String clusterId, InputFie
clusterState.instanceConfigs.add(config);
}
clusterState.clusterConfig = cfgAccessor.getClusterConfig(clusterId);
+
+ if (!inputFields.returnFormat.equals(AssignmentFormat.CurrentStateFormat.name()) &&
Review comment:
This would be hard to maintain if we add more formats in the future. Use enum.valueof and try-catch could be easier.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -277,7 +296,26 @@ private AssignmentResult computeOptimalAssignmentForResources(InputFields inputF
result);
}
- return result;
+ if (inputFields.returnFormat.equals(AssignmentFormat.CurrentStateFormat.name())) {
+ return changeToCurrentStateFormat(result);
+ } else {
+ return result;
+ }
+ }
+
+ // IdealState format : Map of resource -> partition -> instance -> state. (default)
+ // CurrentState format : Map of instance -> resource -> partition -> state.
+ private AssignmentResult changeToCurrentStateFormat(AssignmentResult idealStateFormatResult) {
Review comment:
AssignmentResult is now more complicated, it may have different information. It would be best if we can return this information in the response. Considering if users are going to async call and the handling logic does not know what the original requested format is.
But either way, in the server code here, we should keep tracking what format the AssignmentResult object is using. Either adding a field (indicating format) or child classes (cleaner separate the logic but more code) is fine.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -277,7 +296,26 @@ private AssignmentResult computeOptimalAssignmentForResources(InputFields inputF
result);
}
- return result;
+ if (inputFields.returnFormat.equals(AssignmentFormat.CurrentStateFormat.name())) {
+ return changeToCurrentStateFormat(result);
+ } else {
+ return result;
Review comment:
Same reason as the comment above, "else" here is too wide considering if more enum items are going to be added. Switch with the default condition would help to avoid potential bugs in the future.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -277,7 +296,26 @@ private AssignmentResult computeOptimalAssignmentForResources(InputFields inputF
result);
}
- return result;
+ if (inputFields.returnFormat.equals(AssignmentFormat.CurrentStateFormat.name())) {
+ return changeToCurrentStateFormat(result);
+ } else {
+ return result;
Review comment:
And I would suggest letting the changeXXXXXFormat function handle this check. It will just return whatever format requested.
--
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
---------------------------------------------------------------------
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 #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1787:
URL: https://github.com/apache/helix/pull/1787#discussion_r649579050
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -208,6 +220,13 @@ private ClusterState readClusterStateAndValidateInput(String clusterId, InputFie
clusterState.instanceConfigs.add(config);
}
clusterState.clusterConfig = cfgAccessor.getClusterConfig(clusterId);
+
+ if (!inputFields.returnFormat.equals(AssignmentFormat.CurrentStateFormat.name()) &&
Review comment:
TFTR. Updated. JSON parsing will return IllegalArgumentException for invalid input.
--
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
---------------------------------------------------------------------
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 #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1787:
URL: https://github.com/apache/helix/pull/1787#discussion_r647965390
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -104,11 +107,16 @@ public AssignmentResult() {
Map<String, String> swapInstances;
}
+ private static final String IDEAL_STATE_FORMAT = "IdealStateFormat";
+ private static final String CURRENT_STATE_FORMAT = "CurrentStateFormat";
Review comment:
Define the options by enum?
--
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
---------------------------------------------------------------------
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 #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1787:
URL: https://github.com/apache/helix/pull/1787#discussion_r648572907
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -277,7 +296,26 @@ private AssignmentResult computeOptimalAssignmentForResources(InputFields inputF
result);
}
- return result;
+ if (inputFields.returnFormat.equals(AssignmentFormat.CurrentStateFormat.name())) {
+ return changeToCurrentStateFormat(result);
+ } else {
+ return result;
+ }
+ }
+
+ // IdealState format : Map of resource -> partition -> instance -> state. (default)
+ // CurrentState format : Map of instance -> resource -> partition -> state.
+ private AssignmentResult changeToCurrentStateFormat(AssignmentResult idealStateFormatResult) {
Review comment:
I don't think it is really "self-explanatory". If the controller is managing hosts as the "Helix Resource", then the resource name looks exactly the same as a host name. User will need to compare with the participant name or check with the resource name to ensure. And remember we do not require a resource must be named differently from the participant. So even the logic that I described is a guess : )
It is a very corner case, but it worths the effort to ensure our public API is crystal clear.
--
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
---------------------------------------------------------------------
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 #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1787:
URL: https://github.com/apache/helix/pull/1787#discussion_r649578690
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -277,7 +296,26 @@ private AssignmentResult computeOptimalAssignmentForResources(InputFields inputF
result);
}
- return result;
+ if (inputFields.returnFormat.equals(AssignmentFormat.CurrentStateFormat.name())) {
+ return changeToCurrentStateFormat(result);
+ } else {
+ return result;
+ }
+ }
+
+ // IdealState format : Map of resource -> partition -> instance -> state. (default)
+ // CurrentState format : Map of instance -> resource -> partition -> state.
+ private AssignmentResult changeToCurrentStateFormat(AssignmentResult idealStateFormatResult) {
Review comment:
As or offline discussion, it is better to add metadata (response format, user defined filter etc) in response header. Will have another PR for this since it is a separate logic. I will add a TODO for now.
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -277,7 +296,26 @@ private AssignmentResult computeOptimalAssignmentForResources(InputFields inputF
result);
}
- return result;
+ if (inputFields.returnFormat.equals(AssignmentFormat.CurrentStateFormat.name())) {
+ return changeToCurrentStateFormat(result);
+ } else {
+ return result;
+ }
+ }
+
+ // IdealState format : Map of resource -> partition -> instance -> state. (default)
+ // CurrentState format : Map of instance -> resource -> partition -> state.
+ private AssignmentResult changeToCurrentStateFormat(AssignmentResult idealStateFormatResult) {
Review comment:
As our offline discussion, it is better to add metadata (response format, user defined filter etc) in response header. Will have another PR for this since it is a separate logic. I will add a TODO for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #1787: Support currentState format for partitionAssignment
Posted by GitBox <gi...@apache.org>.
xyuanlu commented on a change in pull request #1787:
URL: https://github.com/apache/helix/pull/1787#discussion_r648560726
##########
File path: helix-rest/src/main/java/org/apache/helix/rest/server/resources/helix/ResourceAssignmentOptimizerAccessor.java
##########
@@ -277,7 +296,26 @@ private AssignmentResult computeOptimalAssignmentForResources(InputFields inputF
result);
}
- return result;
+ if (inputFields.returnFormat.equals(AssignmentFormat.CurrentStateFormat.name())) {
+ return changeToCurrentStateFormat(result);
+ } else {
+ return result;
+ }
+ }
+
+ // IdealState format : Map of resource -> partition -> instance -> state. (default)
+ // CurrentState format : Map of instance -> resource -> partition -> state.
+ private AssignmentResult changeToCurrentStateFormat(AssignmentResult idealStateFormatResult) {
Review comment:
TFTR.
I am not sure if we need to include the return format in the response since the response itself is self explanatory. It is clear that this is a IdealState format or a CurrentState format. However, if user is doing async call with some filter, we might want to return that 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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org