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/08/12 05:07:28 UTC
[GitHub] [helix] jiajunwang opened a new pull request #1256: Do not ignore any map or list field key change in the Change Detector.
jiajunwang opened a new pull request #1256:
URL: https://github.com/apache/helix/pull/1256
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1255
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
This PR extends the scope of the non-trimmable elements that are defined in the ResourceChangeDetector. With this change, any key changes in the Map fields or List fields won't be ignored by the change detector. Instead of just fixing the issue that modifying the IS preference list does not trigger automatic rebalance, this change will also help to prevent other potential bugs with the same root cause.
Note this change might increase the rebalancer running in some circumstances. However, given a very explicit allowlist (or blocklist) in the change detector is very expensive to maintain, the current solution trades off the potential performance impact for simplicity.
### Tests
- [X] The following tests are written for this issue:
Modified the related tests:
1. TestWagedRebalancer.testRebalanceOnChanges
2. TestHelixPropoertyTimmer
- [ ] The following is the result of the "mvn test" command on the appropriate module:
Running.
### 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 #1256: Do not ignore any map or list field key change in the Change Detector.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1256:
URL: https://github.com/apache/helix/pull/1256#discussion_r469417240
##########
File path: helix-core/src/main/java/org/apache/helix/controller/changedetector/trimmer/HelixPropertyTrimmer.java
##########
@@ -57,38 +57,49 @@
* @param originalProperty
*/
protected ZNRecord doTrim(T originalProperty) {
+ ZNRecord originalZNRecord = originalProperty.getRecord();
Review comment:
BTW, the ideal solution is to separate the ZK paths for input and output. Then we can simplify or even remove the trim logic as desired.
----------------------------------------------------------------
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 #1256: Do not ignore any map or list field key change in the Change Detector.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1256:
URL: https://github.com/apache/helix/pull/1256#discussion_r469070998
##########
File path: helix-core/src/main/java/org/apache/helix/controller/changedetector/trimmer/HelixPropertyTrimmer.java
##########
@@ -57,38 +57,49 @@
* @param originalProperty
*/
protected ZNRecord doTrim(T originalProperty) {
+ ZNRecord originalZNRecord = originalProperty.getRecord();
Review comment:
Good point. So this is also my first try. FYI, I also tried to extend the field type to include LIST_FIELD_KEY_ONLY. But then I find these 2 solutions are too complicated.
It is correct that putting this logic into the IdealStateTrimmer will immediately fix the problem without too much change. The complex is that in this way, it would be hard to reason the overall logic of all trimmers. Some of them care about values, the others do not. Of course, we can add comments. But gradually, it will become harder to understand and maintain.
So I go back and think about why we need these trimmers. Do we need to trim soo much information? I think we don't. The rebalancer will perform well even we detect all keys change (no value, just key's change) for all the properties. And the logic is very clean and concentrate:
1. Keys are not ignored
2. Values are ignored accordingly and this is specified by the getNonTrimmableFields() method.
Then I firstly tried to not trim all keys (including the simple fields). But then I realize it is not optimal, since the simple fields' keys are tightly bounded with the values. And finally, I have the current proposal.
Overall, the main reason for this trade-off is to make the trim logic less diverse and easier to maintain.
----------------------------------------------------------------
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 #1256: Do not ignore any map or list field key change in the Change Detector.
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1256:
URL: https://github.com/apache/helix/pull/1256
----------------------------------------------------------------
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] NealSun96 commented on a change in pull request #1256: Do not ignore any map or list field key change in the Change Detector.
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1256:
URL: https://github.com/apache/helix/pull/1256#discussion_r469416552
##########
File path: helix-core/src/main/java/org/apache/helix/controller/changedetector/trimmer/HelixPropertyTrimmer.java
##########
@@ -57,38 +57,49 @@
* @param originalProperty
*/
protected ZNRecord doTrim(T originalProperty) {
+ ZNRecord originalZNRecord = originalProperty.getRecord();
ZNRecord trimmedZNRecord = new ZNRecord(originalProperty.getId());
- for (Map.Entry<FieldType, Set<String>> fieldEntry : getNonTrimmableFields(
- originalProperty).entrySet()) {
+ Map<FieldType, Set<String>> nonTrimmableFields = getNonTrimmableFields(originalProperty);
+
+ // Ensure the keys of all map fields and list fields are preserved even after the trim.
+ // The keys of list and map fields are relatively stable and contain important information. So
+ // they should not be trimmed.
+ originalZNRecord.getMapFields().keySet().stream()
+ .forEach(key -> trimmedZNRecord.setMapField(key, Collections.EMPTY_MAP));
+ originalZNRecord.getListFields().keySet().stream()
+ .forEach(key -> trimmedZNRecord.setListField(key, Collections.EMPTY_LIST));
Review comment:
This part of code may come as a surprise to future developers that extend this class. When `getNonTrimmableFields()` is provided for overriding, it's natural to think `getNonTrimmableFields()` provides full control on "what fields to keep". This logic here, while necessary, may become debug overhead. It's also hard to change this behavior in the future if it stays in `doTrim()` like so.
I suggest creating another function such as `getNonTrimmableKeysOnly()` and implement it under `HelixPropertyTrimmer.java` to preserve MapField and ListField keys by default; in `doTrim()`, we call that function and preserve keys only. The extended classes may choose to override it (and for now, they shouldn't). This is a clearer approach to creating this abstract class.
----------------------------------------------------------------
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] narendly commented on a change in pull request #1256: Do not ignore any map or list field key change in the Change Detector.
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1256:
URL: https://github.com/apache/helix/pull/1256#discussion_r469046459
##########
File path: helix-core/src/main/java/org/apache/helix/controller/changedetector/trimmer/HelixPropertyTrimmer.java
##########
@@ -57,38 +57,49 @@
* @param originalProperty
*/
protected ZNRecord doTrim(T originalProperty) {
+ ZNRecord originalZNRecord = originalProperty.getRecord();
Review comment:
This is a high-level comment:
Since this is causing an issue for the admin.rebalance method (which essentially just populates the mapFields in the IdealState), my immediate instinct would be that this change could be contained in IdealStateTrimmer.
This way, we contain the scope of changes for IdealStates only - and it should fix the problem we're seeing with resources not being rebalanced. Is there any difficulty with overriding `doTrim()` in IdealStateTrimmer instead of making the change and applying it generally in HelixPropertyTrimmer?
----------------------------------------------------------------
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] narendly commented on a change in pull request #1256: Do not ignore any map or list field key change in the Change Detector.
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1256:
URL: https://github.com/apache/helix/pull/1256#discussion_r469046459
##########
File path: helix-core/src/main/java/org/apache/helix/controller/changedetector/trimmer/HelixPropertyTrimmer.java
##########
@@ -57,38 +57,49 @@
* @param originalProperty
*/
protected ZNRecord doTrim(T originalProperty) {
+ ZNRecord originalZNRecord = originalProperty.getRecord();
Review comment:
This is a high-level comment:
Since this is causing an issue for the admin.rebalance method (which essentially just populates the mapFields in the IdealState), my immediate instinct would be that this change could be contained in IdealStateTrimmer.
This way, we contain the scope of changes for IdealStates only. Is there any difficulty with overriding `doTrim()` in IdealStateTrimmer instead of making the change and applying it generally in HelixPropertyTrimmer?
----------------------------------------------------------------
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 #1256: Do not ignore any map or list field key change in the Change Detector.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1256:
URL: https://github.com/apache/helix/pull/1256#discussion_r469427895
##########
File path: helix-core/src/main/java/org/apache/helix/controller/changedetector/trimmer/HelixPropertyTrimmer.java
##########
@@ -57,38 +57,49 @@
* @param originalProperty
*/
protected ZNRecord doTrim(T originalProperty) {
+ ZNRecord originalZNRecord = originalProperty.getRecord();
ZNRecord trimmedZNRecord = new ZNRecord(originalProperty.getId());
- for (Map.Entry<FieldType, Set<String>> fieldEntry : getNonTrimmableFields(
- originalProperty).entrySet()) {
+ Map<FieldType, Set<String>> nonTrimmableFields = getNonTrimmableFields(originalProperty);
+
+ // Ensure the keys of all map fields and list fields are preserved even after the trim.
+ // The keys of list and map fields are relatively stable and contain important information. So
+ // they should not be trimmed.
+ originalZNRecord.getMapFields().keySet().stream()
+ .forEach(key -> trimmedZNRecord.setMapField(key, Collections.EMPTY_MAP));
+ originalZNRecord.getListFields().keySet().stream()
+ .forEach(key -> trimmedZNRecord.setListField(key, Collections.EMPTY_LIST));
Review comment:
Good idea. It helps to clean up the 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 commented on pull request #1256: Do not ignore any map or list field key change in the Change Detector.
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on pull request #1256:
URL: https://github.com/apache/helix/pull/1256#issuecomment-673636212
This PR is approved and ready to be merged.
Re-run the test, all passed.
----------------------------------------------------------------
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