[GitHub] [helix] jiajunwang commented on a change in pull request #363: Fix the race condition while Helix refresh cluster status cache.

2019-07-30 Thread GitBox 
jiajunwang 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_r308872076
 
 

 ##
 File path: 
helix-core/src/main/java/org/apache/helix/controller/dataproviders/ResourceControllerDataProvider.java
 ##
 @@ -108,18 +111,19 @@ public String getObjName(ExternalView obj) {
 
   public synchronized void refresh(HelixDataAccessor accessor) {
 long startTime = System.currentTimeMillis();
+Set propertyRefreshed = new HashSet<>();
+
+// Refresh base
+super.refresh(accessor, propertyRefreshed);
 
 Review comment:
   I was thinking about keeping the new protected method the same as the 
original one. But on my second thought, I think it is doable, and not 
difficult. Let me have a try.
   Good point.


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

[GitHub] [helix] jiajunwang commented on a change in pull request #363: Fix the race condition while Helix refresh cluster status cache.

2019-07-29 Thread GitBox 
jiajunwang 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_r308541153
 
 

 ##
 File path: 
helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
 ##
 @@ -82,7 +84,7 @@
   private ExecutorService _asyncTasksThreadPool;
 
   // A map recording what data has changed
-  protected Map _propertyDataChangedMap;
+  protected Map 
_propertyDataChangedMap;
 
 Review comment:
   That is not essential. Just to make the code simpler and preventing some of 
the potential bugs.
   Just to confirm, are you suggesting that we convert back to Boolean? 
   


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

[GitHub] [helix] jiajunwang commented on a change in pull request #363: Fix the race condition while Helix refresh cluster status cache.

2019-07-29 Thread GitBox 
jiajunwang 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_r308461347
 
 

 ##
 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 refreshedType) {
 // Refresh raw data
 _clusterConfig = 
accessor.getProperty(accessor.keyBuilder().clusterConfig());
-refreshIdealState(accessor);
-refreshLiveInstances(accessor);
-refreshInstanceConfigs(accessor);
-refreshResourceConfig(accessor);
+refreshIdealState(accessor, refreshedType);
 
 Review comment:
   I thought about this. I think the right way is making it property cache 
class method instead of in the data provider. But as you said, not in this PR.
   The previous cache work is half done. We will need to finish the refactor 
work.


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

[GitHub] [helix] jiajunwang commented on a change in pull request #363: Fix the race condition while Helix refresh cluster status cache.

2019-07-29 Thread GitBox 
jiajunwang 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_r308460992
 
 

 ##
 File path: 
helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
 ##
 @@ -597,7 +611,7 @@ public ClusterConstraints 
getConstraint(ClusterConstraints.ConstraintType type)
* Notify the cache that some part of the cluster data has been changed.
*/
   public void notifyDataChange(HelixConstants.ChangeType changeType) {
 
 Review comment:
   Sure.


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

[GitHub] [helix] jiajunwang commented on a change in pull request #363: Fix the race condition while Helix refresh cluster status cache.

2019-07-29 Thread GitBox 
jiajunwang 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_r308460856
 
 

 ##
 File path: 
helix-core/src/main/java/org/apache/helix/controller/dataproviders/BaseControllerDataProvider.java
 ##
 @@ -217,51 +220,54 @@ public String getObjName(StateModelDefinition obj) {
 _instanceMessagesCache = new InstanceMessagesCache(_clusterName);
   }
 
-  private void refreshIdealState(final HelixDataAccessor accessor) {
-if (_propertyDataChangedMap.get(HelixConstants.ChangeType.IDEAL_STATE)) {
-  _propertyDataChangedMap.put(HelixConstants.ChangeType.IDEAL_STATE, 
false);
-
+  private void refreshIdealState(final HelixDataAccessor accessor,
+  Set refreshedType) {
+if 
(_propertyDataChangedMap.get(HelixConstants.ChangeType.IDEAL_STATE).getAndSet(false))
 {
 
 Review comment:
   getAndSet(false) has only one parameter, easier for the reader to get the 
point.


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