do all peak_qps reporting in a timer task

1 files changed, 41 insertions, 39 deletions

diff --git a/container-search/src/main/java/com/yahoo/prelude/statistics/StatisticsSearcher.java b/container-search/src/main/java/com/yahoo/prelude/statistics/StatisticsSearcher.java
index cb8e2e9c266..c436ce09d33 100644
--- a/container-search/src/main/java/com/yahoo/prelude/statistics/StatisticsSearcher.java
+++ b/container-search/src/main/java/com/yahoo/prelude/statistics/StatisticsSearcher.java
@@ -66,14 +66,14 @@ public class StatisticsSearcher extends Searcher {
     private Value peakQPS; // peak 1s QPS
     private Counter emptyResults; // number of results containing no concrete hits
     private Value hitsPerQuery; // mean number of hits per query
-    private long prevMaxQPSTime; // previous measurement time of QPS
-    private double queriesForQPS = 0.0;
-    private final Object peakQpsLock = new Object();
+
+    private final PeakQpsReporter peakQpsReporter;
+
 
     private Metric metric;
     private Map<String, Metric.Context> chainContexts = new CopyOnWriteHashMap<>();
     private Map<String, Metric.Context> statePageOnlyContexts = new CopyOnWriteHashMap<>();
-    static private java.util.Timer scheduler = new java.util.Timer(true);
+    private java.util.Timer scheduler = new java.util.Timer(true);
 
     private void initEvents(com.yahoo.statistics.Statistics manager, MetricReceiver metricReceiver) {
         queries = new Counter(QUERIES_METRIC, manager, false);
@@ -108,19 +108,36 @@ public class StatisticsSearcher extends Searcher {
             metric.set(ACTIVE_QUERIES_METRIC, searchQueriesInFlight, null);
         }
     }
-    private class PeakQpsCleanup extends java.util.TimerTask {
-        private final Metric.Context metricContext;
-        public PeakQpsCleanup(Metric.Context metricContext) {
-            this.metricContext = metricContext;
+    private class PeakQpsReporter extends java.util.TimerTask {
+        private long prevMaxQPSTime = System.currentTimeMillis();
+        private long queriesForQPS = 0;
+        private Metric.Context metricContext = null;
+        public void setContext(Metric.Context metricContext) {
+            if (this.metricContext == null) {
+                synchronized(this) {
+                    this.metricContext = metricContext;
+                }
+            }
         }
         @Override
         public void run() {
             long now = System.currentTimeMillis();
-            synchronized (peakQpsLock) {
-                if ((now - prevMaxQPSTime) >= (1000)) {
-                    flushPeakQps(now, metricContext);
-                    queriesForQPS = 0.0d;
-                }
+            synchronized (this) {
+                if (metricContext == null) return;
+                flushPeakQps(now);
+            }
+        }
+        private void flushPeakQps(long now) {
+            double ms = (double) (now - prevMaxQPSTime);
+            final double value = ((double)queriesForQPS) / (ms / 1000.0);
+            peakQPS.put(value);
+            metric.set(PEAK_QPS_METRIC, value, metricContext);
+            prevMaxQPSTime = now;
+            queriesForQPS = 0;
+        }
+        public void countQuery() {
+            synchronized (this) {
+                ++queriesForQPS;
             }
         }
     }
@@ -130,38 +147,24 @@ public class StatisticsSearcher extends Searcher {
     }
 
     public StatisticsSearcher(com.yahoo.statistics.Statistics manager, Metric metric, MetricReceiver metricReceiver) {
+        this.peakQpsReporter = new PeakQpsReporter();
         this.metric = metric;
         initEvents(manager, metricReceiver);
+        scheduler.schedule(peakQpsReporter, 1000, 1000);
     }
 
-    public String getMyID() {
-        return (getId().stringValue());
+    @Override
+    public void deconstruct() {
+        scheduler.cancel();
     }
 
-    private void flushPeakQps(long now, Metric.Context metricContext) {
-        double ms = (double) (now - prevMaxQPSTime);
-        final double value = queriesForQPS / (ms / 1000);
-        peakQPS.put(value);
-        metric.set(PEAK_QPS_METRIC, value, metricContext);
-        prevMaxQPSTime = now;
-        if (queriesForQPS > 0) {
-            scheduler.schedule(new PeakQpsCleanup(metricContext), 1000);
-        }
+    public String getMyID() {
+        return (getId().stringValue());
     }
 
-    private void qps(long now, Metric.Context metricContext) {
-        // We can either have peakQpsLock _or_ have prevMaxQpsTime as a volatile
-        // and queriesForQPS as an AtomicInteger. That would lead no locking,
-        // but two memory barriers in the common case. Don't change till we know
-        // that is actually better.
-        synchronized (peakQpsLock) {
-            if ((now - prevMaxQPSTime) >= (1000)) {
-                flushPeakQps(now, metricContext);
-                queriesForQPS = 1.0d;
-            } else {
-                queriesForQPS += 1.0d;
-            }
-        }
+    private void qps(Metric.Context metricContext) {
+        peakQpsReporter.setContext(metricContext);
+        peakQpsReporter.countQuery();
     }
 
     private Metric.Context getChainMetricContext(String chainName) {
@@ -187,7 +190,7 @@ public class StatisticsSearcher extends Searcher {
         incrQueryCount(metricContext);
         logQuery(query);
         long start = System.currentTimeMillis(); // Start time, in millisecs.
-        qps(start, metricContext);
+        qps(metricContext);
         Result result;
         //handle exceptions thrown below in searchers
         try {
@@ -197,7 +200,6 @@ public class StatisticsSearcher extends Searcher {
             throw e;
         }
 
-
         long end = System.currentTimeMillis(); // Start time, in millisecs.
         long latency = end - start;
         if (latency >= 0) {