[GitHub] [helix] jiajunwang commented on a change in pull request #1580: Update the internal fields of the Data Providr when overriding the data through set methods.

[GitBox <gi...@apache.org>]

jiajunwang commented on a change in pull request #1580:
URL: https://github.com/apache/helix/pull/1580#discussion_r539711223



##########
File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
##########
@@ -384,8 +384,8 @@ public synchronized void refresh(HelixDataAccessor accessor) {
     _instanceMessagesCache.updateRelayMessages(_liveInstanceCache.getPropertyMap(),
         _currentStateCache.getParticipantStatesMap());
 
-    updateIdealRuleMap();
-    updateDisabledInstances();
+    updateIdealRuleMap(getClusterConfig());

Review comment:
       Because setClusterConfig() is where you update _clusterConfig. And here, it is using it as an input.
   It is a good practice that we don't refer to a private field directly in a private method. Note that the main reason no one calls this method to update for now is that we hide it too deep, so no one remembers. If the parameters are explicitly passed, meaning input and output are clearly divided, then we avoid potential bugs.
   So, IMO this is tightly 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