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

[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