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/01/09 07:21:23 UTC
[GitHub] [helix] jiajunwang opened a new pull request #1607: Fix the IdealStateBuilder to set the correct instance group tag.
jiajunwang opened a new pull request #1607:
URL: https://github.com/apache/helix/pull/1607
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1606
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
This PR aims to simply fix the issue that IdealStateBuilder class ignores the input instance group tag.
### Tests
- [X] The following tests are written for this issue:
TestIdealStateBuilder
- [ ] The following is the result of the "mvn test" command on the appropriate module:
See the CI test result.
### 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 merged pull request #1607: Fix the IdealStateBuilder to set the correct instance group tag.
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1607:
URL: https://github.com/apache/helix/pull/1607
----------------------------------------------------------------
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 #1607: Fix the IdealStateBuilder to set the correct instance group tag.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1607:
URL: https://github.com/apache/helix/pull/1607#discussion_r555320912
##########
File path: helix-core/src/main/java/org/apache/helix/model/builder/IdealStateBuilder.java
##########
@@ -85,12 +87,13 @@
* A constraint that limits the maximum number of partitions per Node.
*/
private int maxPartitionsPerNode;
+
/**
* Allocate the resource to nodes that are tagged with a specific "nodeGroup"
* name. By default a resource will be allocated to all nodes registered to
* the cluster.
*/
- private String nodeGroup = "*";
+ private String nodeGroup = null;
Review comment:
The original code is not fitting the comment. The reason why it is not failing is that the value is not passed.
Please also help to double-check the code. I think "*" will cause the resource to be assigned onto the node with "*" tag. I don't see any wildcard fitting logic in the rebalancer. null is the safest option.
----------------------------------------------------------------
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] pkuwm commented on a change in pull request #1607: Fix the IdealStateBuilder to set the correct instance group tag.
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1607:
URL: https://github.com/apache/helix/pull/1607#discussion_r554467306
##########
File path: helix-core/src/main/java/org/apache/helix/model/builder/IdealStateBuilder.java
##########
@@ -85,12 +87,13 @@
* A constraint that limits the maximum number of partitions per Node.
*/
private int maxPartitionsPerNode;
+
/**
* Allocate the resource to nodes that are tagged with a specific "nodeGroup"
* name. By default a resource will be allocated to all nodes registered to
* the cluster.
*/
- private String nodeGroup = "*";
+ private String nodeGroup = null;
Review comment:
Does it still apply to the java doc "By default a resource will be allocated to all nodes" - do we need to change the java doc?
----------------------------------------------------------------
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] pkuwm commented on a change in pull request #1607: Fix the IdealStateBuilder to set the correct instance group tag.
Posted by GitBox <gi...@apache.org>.
pkuwm commented on a change in pull request #1607:
URL: https://github.com/apache/helix/pull/1607#discussion_r554467306
##########
File path: helix-core/src/main/java/org/apache/helix/model/builder/IdealStateBuilder.java
##########
@@ -85,12 +87,13 @@
* A constraint that limits the maximum number of partitions per Node.
*/
private int maxPartitionsPerNode;
+
/**
* Allocate the resource to nodes that are tagged with a specific "nodeGroup"
* name. By default a resource will be allocated to all nodes registered to
* the cluster.
*/
- private String nodeGroup = "*";
+ private String nodeGroup = null;
Review comment:
Does it still apply to the java doc "By default a resource will be allocated to all nodes" - do we need to change the java doc?
----------------------------------------------------------------
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 pull request #1607: Fix the IdealStateBuilder to set the correct instance group tag.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1607:
URL: https://github.com/apache/helix/pull/1607#issuecomment-759088744
> https://github.com/apache/helix/actions/runs/478730540/workflow, some test failed? Is it related to this change?
No, I have tested without this PR. Same error.
In addition, as I search code, this change is not impacting any other tests except TestIdealStateBuilder in any way.
----------------------------------------------------------------
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 pull request #1607: Fix the IdealStateBuilder to set the correct instance group tag.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1607:
URL: https://github.com/apache/helix/pull/1607#issuecomment-759088063
Approved by @alirezazamani and @dasahcc.
I will merge this PR since the existing test failure is in the master branch already. We will fix that issue separately.
----------------------------------------------------------------
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] kaisun2000 commented on pull request #1607: Fix the IdealStateBuilder to set the correct instance group tag.
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1607:
URL: https://github.com/apache/helix/pull/1607#issuecomment-758899642
https://github.com/apache/helix/actions/runs/478730540/workflow, some test failed? Is it related to this change?
----------------------------------------------------------------
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