[helix] branch master updated: Fix static instance bug in DistViewAggregatorStateModel (#2124)

[zh...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

zhangmeng pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/helix.git


The following commit(s) were added to refs/heads/master by this push:
     new 5909a8a36 Fix static instance bug in DistViewAggregatorStateModel (#2124)
5909a8a36 is described below

commit 5909a8a363499489d545ff4ddf8464140f56076c
Author: Qi (Quincy) Qu <qq...@linkedin.com>
AuthorDate: Tue May 31 12:35:57 2022 -0700

    Fix static instance bug in DistViewAggregatorStateModel (#2124)
    
    Change to use non-static reference in statemodel
---
 .../statemodel/DistViewAggregatorStateModel.java   | 47 +++++++++-------------
 .../view/integration/TestHelixViewAggregator.java  |  1 +
 2 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/helix-view-aggregator/src/main/java/org/apache/helix/view/statemodel/DistViewAggregatorStateModel.java b/helix-view-aggregator/src/main/java/org/apache/helix/view/statemodel/DistViewAggregatorStateModel.java
index 1ccf52d04..7ac20b0da 100644
--- a/helix-view-aggregator/src/main/java/org/apache/helix/view/statemodel/DistViewAggregatorStateModel.java
+++ b/helix-view-aggregator/src/main/java/org/apache/helix/view/statemodel/DistViewAggregatorStateModel.java
@@ -32,8 +32,7 @@ import org.slf4j.LoggerFactory;
 })
 public class DistViewAggregatorStateModel extends AbstractHelixLeaderStandbyStateModel {
   private final static Logger logger = LoggerFactory.getLogger(DistViewAggregatorStateModel.class);
-  private final static Object stateTransitionLock = new Object();
-  private static HelixViewAggregator _aggregator;
+  private HelixViewAggregator _aggregator;
 
   public DistViewAggregatorStateModel(String zkAddr) {
     super(zkAddr);
@@ -48,25 +47,22 @@ public class DistViewAggregatorStateModel extends AbstractHelixLeaderStandbyStat
   public void onBecomeLeaderFromStandby(Message message, NotificationContext context)
       throws Exception {
     String viewClusterName = message.getPartitionName();
-
-    synchronized (stateTransitionLock) {
-      if (_aggregator != null) {
-        logger.warn("Aggregator already exists for view cluster {}: {}: cleaning it up.",
-            viewClusterName, _aggregator.getAggregatorInstanceName());
-        reset();
-      }
-      logger.info("Creating new HelixViewAggregator for view cluster {}", viewClusterName);
-      try {
-        _aggregator = new HelixViewAggregator(viewClusterName, _zkAddr);
-        _aggregator.start();
-      } catch (Exception e) {
-        logger.error("Aggregator failed to become leader from stand by for view cluster {}",
-            viewClusterName, e);
-        reset();
-        throw e;
-      }
-      logStateTransition("STANDBY", "LEADER", message.getPartitionName(), message.getTgtName());
+    if (_aggregator != null) {
+      logger.warn("Aggregator already exists for view cluster {}: {}: cleaning it up.",
+          viewClusterName, _aggregator.getAggregatorInstanceName());
+      reset();
+    }
+    logger.info("Creating new HelixViewAggregator for view cluster {}", viewClusterName);
+    try {
+      _aggregator = new HelixViewAggregator(viewClusterName, _zkAddr);
+      _aggregator.start();
+    } catch (Exception e) {
+      logger.error("Aggregator failed to become leader from stand by for view cluster {}",
+          viewClusterName, e);
+      reset();
+      throw e;
     }
+    logStateTransition("STANDBY", "LEADER", message.getPartitionName(), message.getTgtName());
 
   }
 
@@ -86,15 +82,12 @@ public class DistViewAggregatorStateModel extends AbstractHelixLeaderStandbyStat
     logStateTransition("OFFLINE", "DROPPED", message.getPartitionName(), message.getTgtName());
   }
 
-
   @Override
   public void reset() {
-    synchronized (stateTransitionLock) {
-      if (_aggregator != null) {
-        logger.error("Resetting view aggregator {}", _aggregator.getAggregatorInstanceName());
-        _aggregator.shutdown();
-        _aggregator = null;
-      }
+    if (_aggregator != null) {
+      logger.info("Resetting view aggregator {}", _aggregator.getAggregatorInstanceName());
+      _aggregator.shutdown();
+      _aggregator = null;
     }
   }
 
diff --git a/helix-view-aggregator/src/test/java/org/apache/helix/view/integration/TestHelixViewAggregator.java b/helix-view-aggregator/src/test/java/org/apache/helix/view/integration/TestHelixViewAggregator.java
index 9ffb90666..adf16ec33 100644
--- a/helix-view-aggregator/src/test/java/org/apache/helix/view/integration/TestHelixViewAggregator.java
+++ b/helix-view-aggregator/src/test/java/org/apache/helix/view/integration/TestHelixViewAggregator.java
@@ -56,6 +56,7 @@ public class TestHelixViewAggregator extends ViewAggregatorIntegrationTestBase {
   private HelixAdmin _helixAdmin;
   private MockViewClusterSpectator _monitor;
   private Set<String> _allResources = new HashSet<>();
+  // TODO: add test coverage on multiple statemodel instances for different view clusters
   private DistViewAggregatorStateModel _viewAggregatorStateModel;
 
   @BeforeClass