[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.

[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