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 some try-finally statements #30880

Merged
merged 5 commits into from
May 29, 2018
+71 −87
Merged

Replace some try-finally statements #30880

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
87ed84b
Replace some try-finally statements
May 25, 2018
1726752
Merge branch 'master' into tryWithResources
May 25, 2018
a44f8f3
Merge branch 'master' into tryWithResources
May 26, 2018
1663e85
Merge branch 'master' into tryWithResources
May 27, 2018
295a2d1
Merge branch 'master' into tryWithResources
May 28, 2018
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
10 changes: 3 additions & 7 deletions server/src/main/java/org/elasticsearch/common/geo/parsers/GeoWKTParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,9 @@
*/
package org.elasticsearch.common.geo.parsers;

import org.locationtech.jts.geom.Coordinate;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.geo.GeoPoint;
import org.elasticsearch.common.geo.GeoShapeType;

import java.io.StringReader;
import org.elasticsearch.common.geo.builders.CoordinatesBuilder;
import org.elasticsearch.common.geo.builders.EnvelopeBuilder;
import org.elasticsearch.common.geo.builders.GeometryCollectionBuilder;
Expand All @@ -37,9 +34,11 @@
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.mapper.GeoShapeFieldMapper;
import org.locationtech.jts.geom.Coordinate;

import java.io.IOException;
import java.io.StreamTokenizer;
import java.io.StringReader;
import java.util.List;

/**
Expand Down Expand Up @@ -77,8 +76,7 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha
public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoShapeType shapeType,
final GeoShapeFieldMapper shapeMapper)
throws IOException, ElasticsearchParseException {
StringReader reader = new StringReader(parser.text());
try {
try (StringReader reader = new StringReader(parser.text())) {
boolean ignoreZValue = (shapeMapper != null && shapeMapper.ignoreZValue().value() == true);
// setup the tokenizer; configured to read words w/o numbers
StreamTokenizer tokenizer = new StreamTokenizer(reader);
Expand All @@ -95,8 +93,6 @@ public static ShapeBuilder parseExpectedType(XContentParser parser, final GeoSha
ShapeBuilder builder = parseGeometry(tokenizer, shapeType, ignoreZValue);
checkEOF(tokenizer);
return builder;
} finally {
reader.close();
}
}

Expand Down
17 changes: 4 additions & 13 deletions server/src/main/java/org/elasticsearch/index/analysis/Analysis.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,8 @@ public static List<String> getWordList(Environment env, Settings settings, Strin

final Path path = env.configFile().resolve(wordListPath);

try (BufferedReader reader = Files.newBufferedReader(path, StandardCharsets.UTF_8)) {
return loadWordList(reader, "#");
try {
return loadWordList(path, "#");
} catch (CharacterCodingException ex) {
String message = String.format(Locale.ROOT,
"Unsupported character encoding detected while reading %s_path: %s - files must be UTF-8 encoded",
Expand All @@ -247,15 +247,9 @@ public static List<String> getWordList(Environment env, Settings settings, Strin
}
}

public static List<String> loadWordList(Reader reader, String comment) throws IOException {
private static List<String> loadWordList(Path path, String comment) throws IOException {
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this method should be public. If not, we can create the reader inside.

final List<String> result = new ArrayList<>();
BufferedReader br = null;
try {
if (reader instanceof BufferedReader) {
br = (BufferedReader) reader;
} else {
br = new BufferedReader(reader);
}
try (BufferedReader br = Files.newBufferedReader(path, StandardCharsets.UTF_8)) {
String word;
while ((word = br.readLine()) != null) {
if (!Strings.hasText(word)) {
Expand All @@ -265,9 +259,6 @@ public static List<String> loadWordList(Reader reader, String comment) throws IO
result.add(word.trim());
}
}
} finally {
if (br != null)
br.close();
}
return result;
}
Expand Down
4 changes: 0 additions & 4 deletions server/src/main/java/org/elasticsearch/index/engine/Engine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1424,10 +1424,6 @@ public DocIdAndVersion docIdAndVersion() {

@Override
public void close() {
release();
}

public void release() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced all calls to release() with close().

Releasables.close(searcher);
}
}
Expand Down
4 changes: 2 additions & 2 deletions server/src/main/java/org/elasticsearch/index/get/ShardGetService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ private GetResult innerGet(String type, String id, String[] gFields, boolean rea
get = indexShard.get(new Engine.Get(realtime, readFromTranslog, type, id, uidTerm)
.version(version).versionType(versionType));
if (get.exists() == false) {
get.release();
get.close();
}
}
}
Expand All @@ -172,7 +172,7 @@ private GetResult innerGet(String type, String id, String[] gFields, boolean rea
// break between having loaded it from translog (so we only have _source), and having a document to load
return innerGetLoadFromStoredFields(type, id, gFields, fetchSourceContext, get, mapperService);
} finally {
get.release();
get.close();
}
}

Expand Down
13 changes: 5 additions & 8 deletions server/src/main/java/org/elasticsearch/index/termvectors/TermVectorsService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,6 @@ static TermVectorsResponse getTermVectors(IndexShard indexShard, TermVectorsRequ
termVectorsResponse.setExists(false);
return termVectorsResponse;
}
Engine.GetResult get = indexShard.get(new Engine.Get(request.realtime(), false, request.type(), request.id(), uidTerm)
.version(request.version()).versionType(request.versionType()));

Fields termVectorsByField = null;
AggregatedDfs dfs = null;
Expand All @@ -97,8 +95,9 @@ static TermVectorsResponse getTermVectors(IndexShard indexShard, TermVectorsRequ
handleFieldWildcards(indexShard, request);
}

final Engine.Searcher searcher = indexShard.acquireSearcher("term_vector");
try {
try (Engine.GetResult get = indexShard.get(new Engine.Get(request.realtime(), false, request.type(), request.id(), uidTerm)
.version(request.version()).versionType(request.versionType()));
Engine.Searcher searcher = indexShard.acquireSearcher("term_vector")) {
Fields topLevelFields = MultiFields.getFields(get.searcher() != null ? get.searcher().reader() : searcher.reader());
DocIdAndVersion docIdAndVersion = get.docIdAndVersion();
/* from an artificial document */
Expand Down Expand Up @@ -143,14 +142,12 @@ else if (docIdAndVersion != null) {
}
}
// write term vectors
termVectorsResponse.setFields(termVectorsByField, request.selectedFields(), request.getFlags(), topLevelFields, dfs, termVectorsFilter);
termVectorsResponse.setFields(termVectorsByField, request.selectedFields(), request.getFlags(), topLevelFields, dfs,
termVectorsFilter);
}
termVectorsResponse.setTookInMillis(TimeUnit.NANOSECONDS.toMillis(nanoTimeSupplier.getAsLong() - startTime));
} catch (Exception ex) {
throw new ElasticsearchException("failed to execute term vector request", ex);
} finally {
searcher.close();
get.release();
}
return termVectorsResponse;
}
Expand Down
17 changes: 7 additions & 10 deletions 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 @@ -21,7 +21,6 @@

import org.apache.lucene.search.FieldDoc;
import org.apache.lucene.search.TopDocs;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.action.ActionListener;
Expand All @@ -39,6 +38,7 @@
import org.elasticsearch.common.util.concurrent.AbstractRunnable;
import org.elasticsearch.common.util.concurrent.ConcurrentCollections;
import org.elasticsearch.common.util.concurrent.ConcurrentMapLong;
import org.elasticsearch.core.internal.io.IOUtils;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.index.IndexSettings;
Expand Down Expand Up @@ -92,8 +92,8 @@
import org.elasticsearch.search.sort.SortBuilder;
import org.elasticsearch.search.suggest.Suggest;
import org.elasticsearch.search.suggest.completion.CompletionSuggestion;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.threadpool.Scheduler.Cancellable;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.threadpool.ThreadPool.Names;
import org.elasticsearch.transport.TransportRequest;

Expand Down Expand Up @@ -646,20 +646,17 @@ private void freeAllContextForIndex(Index index) {


public boolean freeContext(long id) {
final SearchContext context = removeContext(id);
if (context != null) {
assert context.refCount() > 0 : " refCount must be > 0: " + context.refCount();
try {
try (SearchContext context = removeContext(id)) {
if (context != null) {
assert context.refCount() > 0 : " refCount must be > 0: " + context.refCount();
context.indexShard().getSearchOperationListener().onFreeContext(context);
if (context.scrollContext() != null) {
context.indexShard().getSearchOperationListener().onFreeScrollContext(context);
}
} finally {
context.close();
return true;
}
return true;
return false;
}
return false;
}

public void freeAllScrollContexts() {
Expand Down
13 changes: 9 additions & 4 deletions server/src/test/java/org/elasticsearch/action/termvectors/MultiTermVectorsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.lucene.index.Fields;
import org.apache.lucene.index.Terms;
import org.apache.lucene.index.TermsEnum;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.action.admin.indices.alias.Alias;
import org.elasticsearch.common.lucene.uid.Versions;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -111,7 +112,8 @@ public void testMultiTermVectorsWithVersion() throws Exception {
checkTermTexts(response.getResponses()[1].getResponse().getFields().terms("field"), new String[]{"value1"});
assertThat(response.getResponses()[2].getFailure(), notNullValue());
assertThat(response.getResponses()[2].getFailure().getId(), equalTo("1"));
assertThat(response.getResponses()[2].getFailure().getCause(), instanceOf(VersionConflictEngineException.class));
assertThat(response.getResponses()[2].getFailure().getCause(), instanceOf(ElasticsearchException.class));
assertThat(response.getResponses()[2].getFailure().getCause().getCause(), instanceOf(VersionConflictEngineException.class));

//Version from Lucene index
refresh();
Expand All @@ -132,7 +134,8 @@ public void testMultiTermVectorsWithVersion() throws Exception {
checkTermTexts(response.getResponses()[1].getResponse().getFields().terms("field"), new String[]{"value1"});
assertThat(response.getResponses()[2].getFailure(), notNullValue());
assertThat(response.getResponses()[2].getFailure().getId(), equalTo("1"));
assertThat(response.getResponses()[2].getFailure().getCause(), instanceOf(VersionConflictEngineException.class));
assertThat(response.getResponses()[2].getFailure().getCause(), instanceOf(ElasticsearchException.class));
assertThat(response.getResponses()[2].getFailure().getCause().getCause(), instanceOf(VersionConflictEngineException.class));


for (int i = 0; i < 3; i++) {
Expand All @@ -155,7 +158,8 @@ public void testMultiTermVectorsWithVersion() throws Exception {
assertThat(response.getResponses()[1].getFailure(), notNullValue());
assertThat(response.getResponses()[1].getFailure().getId(), equalTo("2"));
assertThat(response.getResponses()[1].getIndex(), equalTo("test"));
assertThat(response.getResponses()[1].getFailure().getCause(), instanceOf(VersionConflictEngineException.class));
assertThat(response.getResponses()[1].getFailure().getCause(), instanceOf(ElasticsearchException.class));
assertThat(response.getResponses()[1].getFailure().getCause().getCause(), instanceOf(VersionConflictEngineException.class));
assertThat(response.getResponses()[2].getId(), equalTo("2"));
assertThat(response.getResponses()[2].getIndex(), equalTo("test"));
assertThat(response.getResponses()[2].getFailure(), nullValue());
Expand All @@ -180,7 +184,8 @@ public void testMultiTermVectorsWithVersion() throws Exception {
assertThat(response.getResponses()[1].getFailure(), notNullValue());
assertThat(response.getResponses()[1].getFailure().getId(), equalTo("2"));
assertThat(response.getResponses()[1].getIndex(), equalTo("test"));
assertThat(response.getResponses()[1].getFailure().getCause(), instanceOf(VersionConflictEngineException.class));
assertThat(response.getResponses()[1].getFailure().getCause(), instanceOf(ElasticsearchException.class));
assertThat(response.getResponses()[1].getFailure().getCause().getCause(), instanceOf(VersionConflictEngineException.class));
assertThat(response.getResponses()[2].getId(), equalTo("2"));
assertThat(response.getResponses()[2].getIndex(), equalTo("test"));
assertThat(response.getResponses()[2].getFailure(), nullValue());
Expand Down
60 changes: 30 additions & 30 deletions server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
import com.carrotsearch.randomizedtesting.generators.RandomNumbers;

import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
Expand Down Expand Up @@ -793,7 +794,7 @@ public void testConcurrentGetAndFlush() throws Exception {
while (flushFinished.get() == false) {
Engine.GetResult previousGetResult = latestGetResult.get();
if (previousGetResult != null) {
previousGetResult.release();
previousGetResult.close();
}
latestGetResult.set(engine.get(newGet(true, doc), searcherFactory));
if (latestGetResult.get().exists() == false) {
Expand All @@ -807,7 +808,7 @@ public void testConcurrentGetAndFlush() throws Exception {
flushFinished.set(true);
getThread.join();
assertTrue(latestGetResult.get().exists());
latestGetResult.get().release();
latestGetResult.get().close();
}

public void testSimpleOperations() throws Exception {
Expand All @@ -830,21 +831,20 @@ public void testSimpleOperations() throws Exception {
searchResult.close();

// but, not there non realtime
Engine.GetResult getResult = engine.get(newGet(false, doc), searcherFactory);
assertThat(getResult.exists(), equalTo(false));
getResult.release();
try (Engine.GetResult getResult = engine.get(newGet(false, doc), searcherFactory)) {
assertThat(getResult.exists(), equalTo(false));
}

// but, we can still get it (in realtime)
getResult = engine.get(newGet(true, doc), searcherFactory);
assertThat(getResult.exists(), equalTo(true));
assertThat(getResult.docIdAndVersion(), notNullValue());
getResult.release();
try (Engine.GetResult getResult = engine.get(newGet(true, doc), searcherFactory)) {
assertThat(getResult.exists(), equalTo(true));
assertThat(getResult.docIdAndVersion(), notNullValue());
}

// but not real time is not yet visible
getResult = engine.get(newGet(false, doc), searcherFactory);
assertThat(getResult.exists(), equalTo(false));
getResult.release();

try (Engine.GetResult getResult = engine.get(newGet(false, doc), searcherFactory)) {
assertThat(getResult.exists(), equalTo(false));
}

// refresh and it should be there
engine.refresh("test");
Expand All @@ -856,10 +856,10 @@ public void testSimpleOperations() throws Exception {
searchResult.close();

// also in non realtime
getResult = engine.get(newGet(false, doc), searcherFactory);
assertThat(getResult.exists(), equalTo(true));
assertThat(getResult.docIdAndVersion(), notNullValue());
getResult.release();
try (Engine.GetResult getResult = engine.get(newGet(false, doc), searcherFactory)) {
assertThat(getResult.exists(), equalTo(true));
assertThat(getResult.docIdAndVersion(), notNullValue());
}

// now do an update
document = testDocument();
Expand All @@ -876,10 +876,10 @@ public void testSimpleOperations() throws Exception {
searchResult.close();

// but, we can still get it (in realtime)
getResult = engine.get(newGet(true, doc), searcherFactory);
assertThat(getResult.exists(), equalTo(true));
assertThat(getResult.docIdAndVersion(), notNullValue());
getResult.release();
try (Engine.GetResult getResult = engine.get(newGet(true, doc), searcherFactory)) {
assertThat(getResult.exists(), equalTo(true));
assertThat(getResult.docIdAndVersion(), notNullValue());
}

// refresh and it should be updated
engine.refresh("test");
Expand All @@ -901,9 +901,9 @@ public void testSimpleOperations() throws Exception {
searchResult.close();

// but, get should not see it (in realtime)
getResult = engine.get(newGet(true, doc), searcherFactory);
assertThat(getResult.exists(), equalTo(false));
getResult.release();
try (Engine.GetResult getResult = engine.get(newGet(true, doc), searcherFactory)) {
assertThat(getResult.exists(), equalTo(false));
}

// refresh and it should be deleted
engine.refresh("test");
Expand Down Expand Up @@ -941,10 +941,10 @@ public void testSimpleOperations() throws Exception {
engine.flush();

// and, verify get (in real time)
getResult = engine.get(newGet(true, doc), searcherFactory);
assertThat(getResult.exists(), equalTo(true));
assertThat(getResult.docIdAndVersion(), notNullValue());
getResult.release();
try (Engine.GetResult getResult = engine.get(newGet(true, doc), searcherFactory)) {
assertThat(getResult.exists(), equalTo(true));
assertThat(getResult.docIdAndVersion(), notNullValue());
}

// make sure we can still work with the engine
// now do an update
Expand Down Expand Up @@ -4156,7 +4156,7 @@ public void testSeqNoGenerator() throws IOException {
new Term("_id", parsedDocument.id()),
parsedDocument,
SequenceNumbers.UNASSIGNED_SEQ_NO,
(long) randomIntBetween(1, 8),
randomIntBetween(1, 8),
Versions.MATCH_ANY,
VersionType.INTERNAL,
Engine.Operation.Origin.PRIMARY,
Expand All @@ -4172,7 +4172,7 @@ public void testSeqNoGenerator() throws IOException {
id,
new Term("_id", parsedDocument.id()),
SequenceNumbers.UNASSIGNED_SEQ_NO,
(long) randomIntBetween(1, 8),
randomIntBetween(1, 8),
Versions.MATCH_ANY,
VersionType.INTERNAL,
Engine.Operation.Origin.PRIMARY,
Expand Down
Loading