Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace superfluous usage of Counter with Supplier #39048

Merged
merged 5 commits into from
Feb 21, 2019
Merged

Replace superfluous usage of Counter with Supplier #39048

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8339c2a
Replace superfluous Counter with Supplier
matriv Feb 18, 2019
934bf9a
Fix subclasses and rename method
matriv Feb 18, 2019
45b1afa
Address comments
matriv Feb 18, 2019
260c320
Address comments
matriv Feb 19, 2019
40b56d0
Add 'relative' to variable and method name for more clarity
matriv Feb 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions buildSrc/src/main/resources/forbidden/es-server-signatures.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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#estimatedTimeInMillis instead
org.apache.lucene.search.TimeLimitingCollector#getGlobalTimerThread()
org.apache.lucene.search.TimeLimitingCollector#getGlobalCounter()

Expand Down Expand Up @@ -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)
12 changes: 6 additions & 6 deletions server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -82,13 +81,14 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

final class DefaultSearchContext extends SearchContext {

private final long id;
private final ShardSearchRequest request;
private final SearchShardTarget shardTarget;
private final Counter timeEstimateCounter;
private final Supplier<Long> timeEstimate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use LongSupplier instead

private SearchType searchType;
private final Engine.Searcher engineSearcher;
private final BigArrays bigArrays;
Expand Down Expand Up @@ -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, Supplier<Long> timeEstimate, TimeValue timeout,
FetchPhase fetchPhase, Version minNodeVersion) {
this.id = id;
this.request = request;
Expand All @@ -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.timeEstimate = timeEstimate;
this.timeout = timeout;
this.minNodeVersion = minNodeVersion;
queryShardContext = indexService.newQueryShardContext(request.shardId().id(), searcher.getIndexReader(), request::nowInMillis,
Expand Down Expand Up @@ -804,8 +804,8 @@ public ObjectMapper getObjectMapper(String name) {
}

@Override
public Counter timeEstimateCounter() {
return timeEstimateCounter;
public Supplier<Long> timeEstimate() {
return timeEstimate;
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/search/SearchService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.estimatedTimeInMillis(), timeout,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use threadPool::relativeTimeInMillis

fetchPhase, clusterService.state().nodes().getMinNodeVersion());
boolean success = false;
try {
Expand Down
6 changes: 3 additions & 3 deletions server/src/main/java/org/elasticsearch/search/internal/FilteredSearchContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -57,6 +56,7 @@

import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

public abstract class FilteredSearchContext extends SearchContext {

Expand Down Expand Up @@ -508,8 +508,8 @@ public ObjectMapper getObjectMapper(String name) {
}

@Override
public Counter timeEstimateCounter() {
return in.timeEstimateCounter();
public Supplier<Long> timeEstimate() {
return in.timeEstimate();
}

@Override
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/search/internal/SearchContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -67,6 +66,7 @@
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Supplier;

/**
* This class encapsulates the state needed to execute a search. It holds a reference to the
Expand Down Expand Up @@ -395,7 +395,7 @@ public final boolean hasOnlySuggest() {

public abstract ObjectMapper getObjectMapper(String name);

public abstract Counter timeEstimateCounter();
public abstract Supplier<Long> timeEstimate();

/** Return a view of the additional query collectors that should be run for this context. */
public abstract Map<Class<?>, Collector> queryCollectors();
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/search/internal/SubSearchContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -36,6 +35,7 @@
import org.elasticsearch.search.suggest.SuggestionSearchContext;

import java.util.List;
import java.util.function.Supplier;

public class SubSearchContext extends FilteredSearchContext {

Expand Down Expand Up @@ -354,7 +354,7 @@ public FetchSearchResult fetchResult() {
}

@Override
public Counter timeEstimateCounter() {
public Supplier<Long> timeEstimate() {
throw new UnsupportedOperationException("Not supported");
}

Expand Down
6 changes: 2 additions & 4 deletions server/src/main/java/org/elasticsearch/search/query/QueryPhase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.timeEstimate().get();
final long timeout = searchContext.timeout().millis();
final long maxTime = startTime + timeout;
timeoutRunnable = () -> {
final long time = counter.get();
final long time = searchContext.timeEstimate().get();
if (time > maxTime) {
throw new TimeExceededException();
}
Expand Down
21 changes: 3 additions & 18 deletions server/src/main/java/org/elasticsearch/threadpool/ThreadPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -58,6 +57,7 @@
import java.util.concurrent.ScheduledThreadPoolExecutor;
import java.util.concurrent.ThreadPoolExecutor;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import static java.util.Collections.unmodifiableMap;
Expand Down Expand Up @@ -252,8 +252,8 @@ public long absoluteTimeInMillis() {
return cachedTimeThread.absoluteTimeInMillis();
}

public Counter estimatedTimeInMillisCounter() {
return cachedTimeThread.counter;
public Supplier<Long> estimatedTimeInMillis() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be removed and replaced by ThreadPool::relativeTimeInMillis on the consumer end

return () -> cachedTimeThread.relativeMillis;
}

public ThreadPoolInfo info() {
Expand Down Expand Up @@ -538,7 +538,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;
Expand All @@ -548,7 +547,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);
}

Expand Down Expand Up @@ -581,19 +579,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 {
Expand Down
16 changes: 3 additions & 13 deletions ...ramework/src/main/java/org/elasticsearch/cluster/coordination/DeterministicTaskQueue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -42,6 +41,7 @@
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Function;
import java.util.function.Supplier;

public class DeterministicTaskQueue {

Expand Down Expand Up @@ -304,18 +304,8 @@ public long absoluteTimeInMillis() {
}

@Override
public Counter estimatedTimeInMillisCounter() {
return new Counter() {
@Override
public long addAndGet(long delta) {
throw new UnsupportedOperationException();
}

@Override
public long get() {
return currentTimeMillis;
}
};
public Supplier<Long> estimatedTimeInMillis() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just use :: relativeTimeInMillis on the consumer end.

return () -> currentTimeMillis;
}

@Override
Expand Down
8 changes: 4 additions & 4 deletions test/framework/src/main/java/org/elasticsearch/test/TestSearchContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -64,6 +63,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.function.Supplier;

public class TestSearchContext extends SearchContext {

Expand All @@ -73,7 +73,7 @@ public class TestSearchContext extends SearchContext {
final ThreadPool threadPool;
final Map<Class<?>, Collector> queryCollectors = new HashMap<>();
final IndexShard indexShard;
final Counter timeEstimateCounter = Counter.newCounter();
final Supplier<Long> timeEstimate = () -> 0L;
final QuerySearchResult queryResult = new QuerySearchResult();
final QueryShardContext queryShardContext;
ParsedQuery originalQuery;
Expand Down Expand Up @@ -593,8 +593,8 @@ public void doClose() {
}

@Override
public Counter timeEstimateCounter() {
return timeEstimateCounter;
public Supplier<Long> timeEstimate() {
return timeEstimate;
}

@Override
Expand Down