i3wangyi commented on a change in pull request #363: Fix the race condition while Helix refresh cluster status cache. URL: https://github.com/apache/helix/pull/363#discussion_r308446844
########## File path: helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java ########## @@ -290,12 +296,20 @@ private void updateIdealRuleMap() { } public synchronized void refresh(HelixDataAccessor accessor) { + refresh(accessor, new HashSet<>()); + } + + /** + * @param accessor + * @param refreshedType Record the types that has been updated during the refresh. + */ + protected synchronized void refresh(HelixDataAccessor accessor, Set<HelixConstants.ChangeType> refreshedType) { // Refresh raw data _clusterConfig = accessor.getProperty(accessor.keyBuilder().clusterConfig()); - refreshIdealState(accessor); - refreshLiveInstances(accessor); - refreshInstanceConfigs(accessor); - refreshResourceConfig(accessor); + refreshIdealState(accessor, refreshedType); Review comment: I have some concerns about using an extra Set object to remember what has been refreshed. It's not about correctness or performance, but just feel it's redundant or unnecessary. I noticed the `refreshXXX` method is 99% same only differs in the type, so I guess we can avoid the extra set if we could do the refactor in the second phase with something like this : ``` Set<HelixConstants.ChangeType> refreshProperties(List<HelixConstants.ChangeType> properties) { Set<HelixConstants.ChangeType> refreshed = new HashSet<>(); for (HelixConstants.ChangeType property : properties) { if (_propertyDataChangedMap.get(property).getAndSet(false)) { _proepertyCacheMap.get(property).refresh(accessor); LogUtil.logInfo(logger, getClusterEventId(), String .format("Reloaded %s for cluster %s, %s pipeline. Cnt: %s", property, _clusterName, getPipelineName(), _resourceConfigCache.getPropertyMap().keySet().size())); refreshed.add(property); } } return refreshed; } ``` And the client who uses it like ``` Set<HelixConstants.ChangeType> refreshed = refreshProperties(ImmutableList.of(xxxxx)); ``` ---------------------------------------------------------------- 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: us...@infra.apache.org With regards, Apache Git Services