GEODE-2141: Replace use of List with ConcurrentHashSet for storing various 
stats and listener.
Moved methods stopNotifier_IfEnabledAndRunning and 
startNotifier_IfEnabledAndNotRunning in synchronized
block.

GEODE-2141: Addresing the review comments.

GEODE-2141: Addressing Review Comments
Making addMonitor and remoteMonitor threadsafe.


Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/50320dc4
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/50320dc4
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/50320dc4

Branch: refs/heads/feature/GEODE-2156
Commit: 50320dc4e828c900b6e1ee92c909fa019cae26d0
Parents: c610289
Author: adongre <avin...@ampool.io>
Authored: Sun Nov 27 19:41:48 2016 +0530
Committer: adongre <avin...@ampool.io>
Committed: Fri Dec 2 06:48:35 2016 +0530

----------------------------------------------------------------------
 .../internal/statistics/StatMonitorHandler.java | 34 +++++--------
 .../internal/statistics/StatisticsMonitor.java  | 51 +++++++-------------
 2 files changed, 29 insertions(+), 56 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/50320dc4/geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java
----------------------------------------------------------------------
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java
 
b/geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java
index 0c9583e..170ba49 100755
--- 
a/geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/statistics/StatMonitorHandler.java
@@ -16,12 +16,11 @@ package org.apache.geode.internal.statistics;
 
 import org.apache.geode.SystemFailure;
 import org.apache.geode.distributed.internal.DistributionConfig;
+import org.apache.geode.internal.concurrent.ConcurrentHashSet;
 import org.apache.geode.internal.logging.LogService;
 import org.apache.geode.internal.logging.log4j.LogMarker;
 import org.apache.logging.log4j.Logger;
 
-import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.SynchronousQueue;
 
@@ -38,7 +37,8 @@ public class StatMonitorHandler implements SampleHandler {
   private final boolean enableMonitorThread;
 
   /** The registered monitors */
-  private volatile List<StatisticsMonitor> monitors = 
Collections.<StatisticsMonitor>emptyList();
+  private final ConcurrentHashSet<StatisticsMonitor> monitors =
+      new ConcurrentHashSet<StatisticsMonitor>();
 
   /** Protected by synchronization on this handler instance */
   private volatile StatMonitorNotifier notifier;
@@ -52,11 +52,8 @@ public class StatMonitorHandler implements SampleHandler {
   public boolean addMonitor(StatisticsMonitor monitor) {
     synchronized (this) {
       boolean added = false;
-      List<StatisticsMonitor> oldMonitors = this.monitors;
-      if (!oldMonitors.contains(monitor)) {
-        List<StatisticsMonitor> newMonitors = new 
ArrayList<StatisticsMonitor>(oldMonitors);
-        added = newMonitors.add(monitor);
-        this.monitors = Collections.unmodifiableList(newMonitors);
+      if (!this.monitors.contains(monitor)) {
+        added = this.monitors.add(monitor);
       }
       if (!this.monitors.isEmpty()) {
         startNotifier_IfEnabledAndNotRunning();
@@ -69,11 +66,8 @@ public class StatMonitorHandler implements SampleHandler {
   public boolean removeMonitor(StatisticsMonitor monitor) {
     synchronized (this) {
       boolean removed = false;
-      List<StatisticsMonitor> oldMonitors = this.monitors;
-      if (oldMonitors.contains(monitor)) {
-        List<StatisticsMonitor> newMonitors = new 
ArrayList<StatisticsMonitor>(oldMonitors);
-        removed = newMonitors.remove(monitor);
-        this.monitors = Collections.unmodifiableList(newMonitors);
+      if (this.monitors.contains(monitor)) {
+        removed = this.monitors.remove(monitor);
       }
       if (this.monitors.isEmpty()) {
         stopNotifier_IfEnabledAndRunning();
@@ -110,8 +104,7 @@ public class StatMonitorHandler implements SampleHandler {
   }
 
   private void monitor(final long sampleTimeMillis, final 
List<ResourceInstance> resourceInstance) {
-    List<StatisticsMonitor> currentMonitors = StatMonitorHandler.this.monitors;
-    for (StatisticsMonitor monitor : currentMonitors) {
+    for (StatisticsMonitor monitor : StatMonitorHandler.this.monitors) {
       try {
         monitor.monitor(sampleTimeMillis, resourceInstance);
       } catch (VirtualMachineError e) {
@@ -138,15 +131,13 @@ public class StatMonitorHandler implements SampleHandler {
   public void destroyedResourceInstance(ResourceInstance resourceInstance) {}
 
   /** For testing only */
-  List<StatisticsMonitor> getMonitorsSnapshot() {
-    return Collections.unmodifiableList(this.monitors);
+  ConcurrentHashSet<StatisticsMonitor> getMonitorsSnapshot() {
+    return this.monitors;
   }
 
   /** For testing only */
   StatMonitorNotifier getStatMonitorNotifier() {
-    synchronized (this) {
-      return this.notifier;
-    }
+    return this.notifier;
   }
 
   private void startNotifier_IfEnabledAndNotRunning() {
@@ -228,8 +219,7 @@ public class StatMonitorHandler implements SampleHandler {
             }
           }
           if (working && latestTask != null) {
-            List<StatisticsMonitor> currentMonitors = 
StatMonitorHandler.this.monitors;
-            for (StatisticsMonitor monitor : currentMonitors) {
+            for (StatisticsMonitor monitor : StatMonitorHandler.this.monitors) 
{
               try {
                 monitor.monitor(latestTask.getSampleTimeMillis(),
                     latestTask.getResourceInstances());

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/50320dc4/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsMonitor.java
----------------------------------------------------------------------
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsMonitor.java
 
b/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsMonitor.java
index 0e53557..16f71e2 100755
--- 
a/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsMonitor.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/statistics/StatisticsMonitor.java
@@ -14,8 +14,8 @@
  */
 package org.apache.geode.internal.statistics;
 
-import java.util.ArrayList;
-import java.util.Collections;
+import org.apache.geode.internal.concurrent.ConcurrentHashSet;
+
 import java.util.List;
 
 /**
@@ -29,9 +29,10 @@ public abstract class StatisticsMonitor {
 
   private final Object mutex = new Object();
 
-  private volatile List<StatisticsListener> listeners = 
Collections.<StatisticsListener>emptyList();
+  private final ConcurrentHashSet<StatisticsListener> listeners =
+      new ConcurrentHashSet<StatisticsListener>();
 
-  private volatile List<StatisticId> statisticIds = 
Collections.<StatisticId>emptyList();
+  private final ConcurrentHashSet<StatisticId> statisticIds = new 
ConcurrentHashSet<StatisticId>();
 
   public StatisticsMonitor() {}
 
@@ -39,13 +40,8 @@ public abstract class StatisticsMonitor {
     if (statId == null) {
       throw new NullPointerException("StatisticId is null");
     }
-    synchronized (this.mutex) {
-      List<StatisticId> oldStatisticIds = this.statisticIds;
-      if (!oldStatisticIds.contains(statId)) {
-        List<StatisticId> newStatisticIds = new 
ArrayList<StatisticId>(oldStatisticIds);
-        newStatisticIds.add(statId);
-        this.statisticIds = Collections.unmodifiableList(newStatisticIds);
-      }
+    if (!this.statisticIds.contains(statId)) {
+      this.statisticIds.add(statId);
     }
     return this;
   }
@@ -54,13 +50,8 @@ public abstract class StatisticsMonitor {
     if (statId == null) {
       throw new NullPointerException("StatisticId is null");
     }
-    synchronized (this.mutex) {
-      List<StatisticId> oldStatisticIds = this.statisticIds;
-      if (oldStatisticIds.contains(statId)) {
-        List<StatisticId> newStatisticIds = new 
ArrayList<StatisticId>(oldStatisticIds);
-        newStatisticIds.remove(statId);
-        this.statisticIds = Collections.unmodifiableList(newStatisticIds);
-      }
+    if (this.statisticIds.contains(statId)) {
+      this.statisticIds.remove(statId);
     }
     return this;
   }
@@ -70,11 +61,8 @@ public abstract class StatisticsMonitor {
       throw new NullPointerException("StatisticsListener is null");
     }
     synchronized (this.mutex) {
-      List<StatisticsListener> oldListeners = this.listeners;
-      if (!oldListeners.contains(listener)) {
-        List<StatisticsListener> newListeners = new 
ArrayList<StatisticsListener>(oldListeners);
-        newListeners.add(listener);
-        this.listeners = Collections.unmodifiableList(newListeners);
+      if (!this.listeners.contains(listener)) {
+        this.listeners.add(listener);
         getStatMonitorHandler().addMonitor(this);
       }
     }
@@ -85,18 +73,15 @@ public abstract class StatisticsMonitor {
       throw new NullPointerException("StatisticsListener is null");
     }
     synchronized (this.mutex) {
-      List<StatisticsListener> oldListeners = this.listeners;
-      if (oldListeners.contains(listener)) {
-        List<StatisticsListener> newListeners = new 
ArrayList<StatisticsListener>(oldListeners);
-        newListeners.remove(listener);
-        if (newListeners.isEmpty()) {
+      if (this.listeners.contains(listener)) {
+        this.listeners.remove(listener);
+        if (this.listeners.isEmpty()) {
           try {
             getStatMonitorHandler().removeMonitor(this);
           } catch (IllegalStateException ignore) {
             // sample collector and handlers were closed (ok on removal)
           }
         }
-        this.listeners = Collections.unmodifiableList(newListeners);
       }
     }
   }
@@ -114,15 +99,13 @@ public abstract class StatisticsMonitor {
 
   private final void monitorStatisticIds(long millisTimeStamp,
       List<ResourceInstance> resourceInstances) {
-    List<StatisticId> statisticIdsToMonitor = statisticIds;
-    if (!statisticIdsToMonitor.isEmpty()) {
+    if (!this.statisticIds.isEmpty()) {
       // TODO:
     }
   }
 
   protected final void notifyListeners(StatisticsNotification notification) {
-    List<StatisticsListener> listenersToNotify = this.listeners;
-    for (StatisticsListener listener : listenersToNotify) {
+    for (StatisticsListener listener : this.listeners) {
       listener.handleNotification(notification);
     }
   }
@@ -136,7 +119,7 @@ public abstract class StatisticsMonitor {
   }
 
   /** For testing only */
-  List<StatisticsListener> getStatisticsListenersSnapshot() {
+  ConcurrentHashSet<StatisticsListener> getStatisticsListenersSnapshot() {
     return this.listeners;
   }
 

Reply via email to