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

Jump to bottom

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 3 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#relativeTimeInMillis 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.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 timeEstimate;
Copy link
Member

Choose a reason for hiding this comment

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

I think this name is misleading because we are using a relative notion of time here whereas the name could be construed as an estimate of the absolute time (like a timestamp). In other places we use the name relativeTimeSupplier to make it clear the purpose of the variable, and to avoid a problem if in the future someone were to use this variable for another piece of code to try to get the absolute time. It would also be worthwhile to add a comment explaining why relative time is okay here. It is important to keep the two uses between absolute and relative time absolutely clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we change also the iface method name(): https://github.com/elastic/elasticsearch/pull/39048/files#diff-2410736cbe252badfd50ccbfceebc857R398 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just name it timeSupplier. I also think we should not expose the LongSupplier on SerachContext at all and only provide a method to access the time public long getTime() on SearchContext.java The usage is simple and can just call this method. no need to expose the supplier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chose getTimeInMillis() to be even more clear.

Copy link
Member

Choose a reason for hiding this comment

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

Please include the word relative in the name.

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, LongSupplier 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 LongSupplier 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::relativeTimeInMillis, timeout,
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.LongSupplier;

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 LongSupplier 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.LongSupplier;

/**
* 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 LongSupplier 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.LongSupplier;

public class SubSearchContext extends FilteredSearchContext {

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

@Override
public Counter timeEstimateCounter() {
public LongSupplier 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().getAsLong();
final long timeout = searchContext.timeout().millis();
final long maxTime = startTime + timeout;
timeoutRunnable = () -> {
final long time = counter.get();
final long time = searchContext.timeEstimate().getAsLong();
if (time > maxTime) {
throw new TimeExceededException();
}
Expand Down
20 changes: 0 additions & 20 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 @@ -252,10 +251,6 @@ public long absoluteTimeInMillis() {
return cachedTimeThread.absoluteTimeInMillis();
}

public Counter estimatedTimeInMillisCounter() {
return cachedTimeThread.counter;
}

public ThreadPoolInfo info() {
return threadPoolInfo;
}
Expand Down Expand Up @@ -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;
Expand All @@ -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);
}

Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
16 changes: 0 additions & 16 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 Down Expand Up @@ -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();
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.LongSupplier;

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 LongSupplier 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 LongSupplier timeEstimate() {
return timeEstimate;
}

@Override
Expand Down