Replace superfluous usage of Counter with Supplier (#39048) (#39225)

`Counter` was used as a means of a functional argument to pass the relative cached time before `Supplier` iface was introduced.
elastic · Feb 21, 2019 · 1316825 · 1316825
1 parent be8a531
commit 1316825
Show file tree

Hide file tree

Showing 11 changed files with 22 additions and 61 deletions.
diff --git a/buildSrc/src/main/resources/forbidden/es-server-signatures.txt b/buildSrc/src/main/resources/forbidden/es-server-signatures.txt
@@ -61,7 +61,7 @@ java.nio.channels.FileChannel#read(java.nio.ByteBuffer, long)
 @defaultMessage Use Lucene.parseLenient instead it strips off minor version
 org.apache.lucene.util.Version#parseLeniently(java.lang.String)
 
-@defaultMessage Spawns a new thread which is solely under lucenes control use ThreadPool#estimatedTimeInMillisCounter instead
+@defaultMessage Spawns a new thread which is solely under lucenes control use ThreadPool#relativeTimeInMillis instead
 org.apache.lucene.search.TimeLimitingCollector#getGlobalTimerThread()
 org.apache.lucene.search.TimeLimitingCollector#getGlobalCounter()
 
@@ -146,4 +146,4 @@ org.apache.logging.log4j.Logger#warn(java.lang.Object, java.lang.Throwable)
 org.apache.logging.log4j.Logger#error(java.lang.Object)
 org.apache.logging.log4j.Logger#error(java.lang.Object, java.lang.Throwable)
 org.apache.logging.log4j.Logger#fatal(java.lang.Object)
-org.apache.logging.log4j.Logger#fatal(java.lang.Object, java.lang.Throwable)
+org.apache.logging.log4j.Logger#fatal(java.lang.Object, java.lang.Throwable)
diff --git a/server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java b/server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java
@@ -24,7 +24,6 @@
 import org.apache.lucene.search.Collector;
 import org.apache.lucene.search.FieldDoc;
 import org.apache.lucene.search.Query;
-import org.apache.lucene.util.Counter;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.search.SearchTask;
 import org.elasticsearch.action.search.SearchType;
@@ -82,13 +81,14 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.function.LongSupplier;
 
 final class DefaultSearchContext extends SearchContext {
 
     private final long id;
     private final ShardSearchRequest request;
     private final SearchShardTarget shardTarget;
-    private final Counter timeEstimateCounter;
+    private final LongSupplier relativeTimeSupplier;
     private SearchType searchType;
     private final Engine.Searcher engineSearcher;
     private final BigArrays bigArrays;
@@ -159,7 +159,7 @@ final class DefaultSearchContext extends SearchContext {
 
     DefaultSearchContext(long id, ShardSearchRequest request, SearchShardTarget shardTarget,
                          Engine.Searcher engineSearcher, ClusterService clusterService, IndexService indexService,
-                         IndexShard indexShard, BigArrays bigArrays, Counter timeEstimateCounter, TimeValue timeout,
+                         IndexShard indexShard, BigArrays bigArrays, LongSupplier relativeTimeSupplier, TimeValue timeout,
                          FetchPhase fetchPhase, Version minNodeVersion) {
         this.id = id;
         this.request = request;
@@ -176,7 +176,7 @@ final class DefaultSearchContext extends SearchContext {
         this.indexService = indexService;
         this.clusterService = clusterService;
         this.searcher = new ContextIndexSearcher(engineSearcher, indexService.cache().query(), indexShard.getQueryCachingPolicy());
-        this.timeEstimateCounter = timeEstimateCounter;
+        this.relativeTimeSupplier = relativeTimeSupplier;
         this.timeout = timeout;
         this.minNodeVersion = minNodeVersion;
         queryShardContext = indexService.newQueryShardContext(request.shardId().id(), searcher.getIndexReader(), request::nowInMillis,
@@ -804,8 +804,8 @@ public ObjectMapper getObjectMapper(String name) {
     }
 
     @Override
-    public Counter timeEstimateCounter() {
-        return timeEstimateCounter;
+    public long getRelativeTimeInMillis() {
+        return relativeTimeSupplier.getAsLong();
     }
 
     @Override

diff --git a/server/src/main/java/org/elasticsearch/search/SearchService.java b/server/src/main/java/org/elasticsearch/search/SearchService.java
@@ -648,7 +648,7 @@ private DefaultSearchContext createSearchContext(ShardSearchRequest request, Tim
         Engine.Searcher engineSearcher = indexShard.acquireSearcher(source);
 
         final DefaultSearchContext searchContext = new DefaultSearchContext(idGenerator.incrementAndGet(), request, shardTarget,
-            engineSearcher, clusterService, indexService, indexShard, bigArrays, threadPool.estimatedTimeInMillisCounter(), timeout,
+            engineSearcher, clusterService, indexService, indexShard, bigArrays, threadPool::relativeTimeInMillis, timeout,
             fetchPhase, clusterService.state().nodes().getMinNodeVersion());
         boolean success = false;
         try {

diff --git a/server/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java b/server/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java
@@ -22,7 +22,6 @@
 import org.apache.lucene.search.Collector;
 import org.apache.lucene.search.FieldDoc;
 import org.apache.lucene.search.Query;
-import org.apache.lucene.util.Counter;
 import org.elasticsearch.action.search.SearchTask;
 import org.elasticsearch.action.search.SearchType;
 import org.elasticsearch.common.unit.TimeValue;
@@ -508,8 +507,8 @@ public ObjectMapper getObjectMapper(String name) {
     }
 
     @Override
-    public Counter timeEstimateCounter() {
-        return in.timeEstimateCounter();
+    public long getRelativeTimeInMillis() {
+        return in.getRelativeTimeInMillis();
     }
 
     @Override

diff --git a/server/src/main/java/org/elasticsearch/search/internal/SearchContext.java b/server/src/main/java/org/elasticsearch/search/internal/SearchContext.java
@@ -22,7 +22,6 @@
 import org.apache.lucene.search.Collector;
 import org.apache.lucene.search.FieldDoc;
 import org.apache.lucene.search.Query;
-import org.apache.lucene.util.Counter;
 import org.elasticsearch.action.search.SearchTask;
 import org.elasticsearch.action.search.SearchType;
 import org.elasticsearch.common.Nullable;
@@ -395,7 +394,11 @@ public final boolean hasOnlySuggest() {
 
     public abstract ObjectMapper getObjectMapper(String name);
 
-    public abstract Counter timeEstimateCounter();
+    /**
+     * Returns time in milliseconds that can be used for relative time calculations.
+     * WARN: This is not the epoch time.
+     */
+    public abstract long getRelativeTimeInMillis();
 
     /** Return a view of the additional query collectors that should be run for this context. */
     public abstract Map<Class<?>, Collector> queryCollectors();

diff --git a/server/src/main/java/org/elasticsearch/search/internal/SubSearchContext.java b/server/src/main/java/org/elasticsearch/search/internal/SubSearchContext.java
@@ -19,7 +19,6 @@
 package org.elasticsearch.search.internal;
 
 import org.apache.lucene.search.Query;
-import org.apache.lucene.util.Counter;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.index.query.ParsedQuery;
 import org.elasticsearch.search.aggregations.SearchContextAggregations;
@@ -354,7 +353,7 @@ public FetchSearchResult fetchResult() {
     }
 
     @Override
-    public Counter timeEstimateCounter() {
+    public long getRelativeTimeInMillis() {
         throw new UnsupportedOperationException("Not supported");
     }
 

diff --git a/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java b/server/src/main/java/org/elasticsearch/search/query/QueryPhase.java
@@ -37,7 +37,6 @@
 import org.apache.lucene.search.Sort;
 import org.apache.lucene.search.TopDocs;
 import org.apache.lucene.search.TotalHits;
-import org.apache.lucene.util.Counter;
 import org.elasticsearch.action.search.SearchTask;
 import org.elasticsearch.common.lucene.Lucene;
 import org.elasticsearch.common.lucene.search.TopDocsAndMaxScore;
@@ -213,12 +212,11 @@ static boolean execute(SearchContext searchContext,
 
             final Runnable timeoutRunnable;
             if (timeoutSet) {
-                final Counter counter = searchContext.timeEstimateCounter();
-                final long startTime = counter.get();
+                final long startTime = searchContext.getRelativeTimeInMillis();
                 final long timeout = searchContext.timeout().millis();
                 final long maxTime = startTime + timeout;
                 timeoutRunnable = () -> {
-                    final long time = counter.get();
+                    final long time = searchContext.getRelativeTimeInMillis();
                     if (time > maxTime) {
                         throw new TimeExceededException();
                     }

diff --git a/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java b/server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java
@@ -22,7 +22,6 @@
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.ParameterizedMessage;
-import org.apache.lucene.util.Counter;
 import org.elasticsearch.Version;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.io.stream.StreamInput;
@@ -252,10 +251,6 @@ public long absoluteTimeInMillis() {
         return cachedTimeThread.absoluteTimeInMillis();
     }
 
-    public Counter estimatedTimeInMillisCounter() {
-        return cachedTimeThread.counter;
-    }
-
     public ThreadPoolInfo info() {
         return threadPoolInfo;
     }
@@ -538,7 +533,6 @@ public String toString() {
     static class CachedTimeThread extends Thread {
 
         final long interval;
-        final TimeCounter counter;
         volatile boolean running = true;
         volatile long relativeMillis;
         volatile long absoluteMillis;
@@ -548,7 +542,6 @@ static class CachedTimeThread extends Thread {
             this.interval = interval;
             this.relativeMillis = TimeValue.nsecToMSec(System.nanoTime());
             this.absoluteMillis = System.currentTimeMillis();
-            this.counter = new TimeCounter();
             setDaemon(true);
         }
 
@@ -581,19 +574,6 @@ public void run() {
                 }
             }
         }
-
-        private class TimeCounter extends Counter {
-
-            @Override
-            public long addAndGet(long delta) {
-                throw new UnsupportedOperationException();
-            }
-
-            @Override
-            public long get() {
-                return relativeMillis;
-            }
-        }
     }
 
     static class ExecutorHolder {

diff --git a/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java b/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java
@@ -741,7 +741,7 @@ public void testDeleteNotLost() throws Exception {
                         .getVersion(),
                 equalTo(-1L));
 
-        // ThreadPool.estimatedTimeInMillis has default granularity of 200 msec, so we must sleep at least that long; sleep much longer in
+        // ThreadPool.relativeTimeInMillis has default granularity of 200 msec, so we must sleep at least that long; sleep much longer in
         // case system is busy:
         Thread.sleep(1000);
 

diff --git a/...ramework/src/main/java/org/elasticsearch/cluster/coordination/DeterministicTaskQueue.java b/...ramework/src/main/java/org/elasticsearch/cluster/coordination/DeterministicTaskQueue.java
@@ -22,7 +22,6 @@
 import com.carrotsearch.randomizedtesting.generators.RandomNumbers;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.apache.lucene.util.Counter;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.threadpool.ThreadPool;
@@ -303,21 +302,6 @@ public long absoluteTimeInMillis() {
                 return currentTimeMillis;
             }
 
-            @Override
-            public Counter estimatedTimeInMillisCounter() {
-                return new Counter() {
-                    @Override
-                    public long addAndGet(long delta) {
-                        throw new UnsupportedOperationException();
-                    }
-
-                    @Override
-                    public long get() {
-                        return currentTimeMillis;
-                    }
-                };
-            }
-
             @Override
             public ThreadPoolInfo info() {
                 throw new UnsupportedOperationException();

diff --git a/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java b/test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java
@@ -21,7 +21,6 @@
 import org.apache.lucene.search.Collector;
 import org.apache.lucene.search.FieldDoc;
 import org.apache.lucene.search.Query;
-import org.apache.lucene.util.Counter;
 import org.elasticsearch.action.search.SearchTask;
 import org.elasticsearch.action.search.SearchType;
 import org.elasticsearch.common.unit.TimeValue;
@@ -73,7 +72,6 @@ public class TestSearchContext extends SearchContext {
     final ThreadPool threadPool;
     final Map<Class<?>, Collector> queryCollectors = new HashMap<>();
     final IndexShard indexShard;
-    final Counter timeEstimateCounter = Counter.newCounter();
     final QuerySearchResult queryResult = new QuerySearchResult();
     final QueryShardContext queryShardContext;
     ParsedQuery originalQuery;
@@ -593,8 +591,8 @@ public void doClose() {
     }
 
     @Override
-    public Counter timeEstimateCounter() {
-        return timeEstimateCounter;
+    public long getRelativeTimeInMillis() {
+        return 0L;
     }
 
     @Override