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:
[email protected]
With regards,
Apache Git Services
