Skip to content

Commit

Permalink
Preserve index_uuid when creating QueryShardException
Browse files Browse the repository at this point in the history
As part of elastic#32608 we made sure that the fully qualified index name is taken from the query shard context whenever creating a new `QueryShardException`. That change introduced a regression as instead of setting the entire `Index` object to the exception, which holds index name and index uuid, we ended up setting only the index name (including cluster alias). With this commit we make sure that the index uuid does not get lost and we try to lower the chances that a similar bug makes it in another time. That's done by making `QueryShardContext` return the fully qualified `Index` (which also holds the uuid) rather than only the fully qualified index name.
  • Loading branch information
javanna committed Aug 7, 2018
1 parent 6fe6247 commit c474c74
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 57 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -756,13 +756,12 @@ public BitSetProducer bitsetFilter(Query query) {
@Override
@SuppressWarnings("unchecked")
public <IFD extends IndexFieldData<?>> IFD getForField(MappedFieldType fieldType) {
IndexFieldData.Builder builder = fieldType.fielddataBuilder(shardContext.getFullyQualifiedIndexName());
IndexFieldData.Builder builder = fieldType.fielddataBuilder(shardContext.getFullyQualifiedIndex().getName());
IndexFieldDataCache cache = new IndexFieldDataCache.None();
CircuitBreakerService circuitBreaker = new NoneCircuitBreakerService();
return (IFD) builder.build(shardContext.getIndexSettings(), fieldType, cache, circuitBreaker,
shardContext.getMapperService());
}
};
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public Query existsQuery(QueryShardContext context) {
*/
@Override
public Query termQuery(Object value, @Nullable QueryShardContext context) {
if (isSameIndex(value, context.getFullyQualifiedIndexName())) {
if (isSameIndex(value, context.getFullyQualifiedIndex().getName())) {
return Queries.newMatchAllQuery();
} else {
return Queries.newMatchNoDocsQuery("Index didn't match. Index queried: " + context.index().getName() + " vs. " + value);
Expand All @@ -139,14 +139,14 @@ public Query termsQuery(List values, QueryShardContext context) {
return super.termsQuery(values, context);
}
for (Object value : values) {
if (isSameIndex(value, context.getFullyQualifiedIndexName())) {
if (isSameIndex(value, context.getFullyQualifiedIndex().getName())) {
// No need to OR these clauses - we can only logically be
// running in the context of just one of these index names.
return Queries.newMatchAllQuery();
}
}
// None of the listed index names are this one
return Queries.newMatchNoDocsQuery("Index didn't match. Index queried: " + context.getFullyQualifiedIndexName()
return Queries.newMatchNoDocsQuery("Index didn't match. Index queried: " + context.getFullyQualifiedIndex().getName()
+ " vs. " + values);
}

Expand Down Expand Up @@ -189,5 +189,4 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
protected void doMerge(Mapper mergeWith) {
// nothing to do
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public class QueryShardContext extends QueryRewriteContext {
private String[] types = Strings.EMPTY_ARRAY;
private boolean cachable = true;
private final SetOnce<Boolean> frozen = new SetOnce<>();
private final String fullyQualifiedIndexName;
private final Index fullyQualifiedIndex;

public void setTypes(String... types) {
this.types = types;
Expand Down Expand Up @@ -116,7 +116,8 @@ public QueryShardContext(int shardId, IndexSettings indexSettings, BitsetFilterC
this.indexSettings = indexSettings;
this.reader = reader;
this.clusterAlias = clusterAlias;
this.fullyQualifiedIndexName = RemoteClusterAware.buildRemoteIndexName(clusterAlias, indexSettings.getIndex().getName());
this.fullyQualifiedIndex = new Index(RemoteClusterAware.buildRemoteIndexName(clusterAlias, indexSettings.getIndex().getName()),
indexSettings.getIndex().getUUID());
}

public QueryShardContext(QueryShardContext source) {
Expand Down Expand Up @@ -163,7 +164,7 @@ public BitSetProducer bitsetFilter(Query filter) {
}

public <IFD extends IndexFieldData<?>> IFD getForField(MappedFieldType fieldType) {
return (IFD) indexFieldDataService.apply(fieldType, fullyQualifiedIndexName);
return (IFD) indexFieldDataService.apply(fieldType, fullyQualifiedIndex.getName());
}

public void addNamedQuery(String name, Query query) {
Expand Down Expand Up @@ -275,7 +276,7 @@ public Collection<String> queryTypes() {
public SearchLookup lookup() {
if (lookup == null) {
lookup = new SearchLookup(getMapperService(),
mappedFieldType -> indexFieldDataService.apply(mappedFieldType, fullyQualifiedIndexName), types);
mappedFieldType -> indexFieldDataService.apply(mappedFieldType, fullyQualifiedIndex.getName()), types);
}
return lookup;
}
Expand Down Expand Up @@ -426,9 +427,9 @@ public IndexReader getIndexReader() {
}

/**
* Returns the fully qualified index name including a remote cluster alias if applicable
* Returns the fully qualified index including a remote cluster alias if applicable, and the index uuid
*/
public String getFullyQualifiedIndexName() {
return fullyQualifiedIndexName;
public Index getFullyQualifiedIndex() {
return fullyQualifiedIndex;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,15 @@ public QueryShardException(QueryShardContext context, String msg, Object... args
}

public QueryShardException(QueryShardContext context, String msg, Throwable cause, Object... args) {
super(msg, cause, args);
setIndex(context.getFullyQualifiedIndexName());
this(context.getFullyQualifiedIndex(), msg, cause, args);
}

/**
* This constructor is provided for use in unit tests where a
* {@link QueryShardContext} may not be available
*/
public QueryShardException(Index index, String msg, Throwable cause) {
super(msg, cause);
public QueryShardException(Index index, String msg, Throwable cause, Object... args) {
super(msg, cause, args);
setIndex(index);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
import org.apache.lucene.search.Query;
import org.elasticsearch.Version;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.lucene.search.Queries;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.fielddata.IndexFieldData;
import org.elasticsearch.index.fielddata.plain.AbstractAtomicOrdinalsFieldData;
Expand All @@ -37,6 +39,7 @@
import org.hamcrest.Matchers;

import java.io.IOException;
import java.util.Collections;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
Expand All @@ -49,24 +52,7 @@
public class QueryShardContextTests extends ESTestCase {

public void testFailIfFieldMappingNotFound() {
IndexMetaData.Builder indexMetadataBuilder = new IndexMetaData.Builder("index");
indexMetadataBuilder.settings(Settings.builder().put("index.version.created", Version.CURRENT)
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 1)
);
IndexMetaData indexMetaData = indexMetadataBuilder.build();
IndexSettings indexSettings = new IndexSettings(indexMetaData, Settings.EMPTY);
MapperService mapperService = mock(MapperService.class);
when(mapperService.getIndexSettings()).thenReturn(indexSettings);
when(mapperService.index()).thenReturn(indexMetaData.getIndex());
final long nowInMillis = randomNonNegativeLong();

QueryShardContext context = new QueryShardContext(
0, indexSettings, null, (mappedFieldType, idxName) ->
mappedFieldType.fielddataBuilder(idxName).build(indexSettings, mappedFieldType, null, null, null)
, mapperService, null, null, xContentRegistry(), writableRegistry(), null, null,
() -> nowInMillis, null);

QueryShardContext context = createQueryShardContext(IndexMetaData.INDEX_UUID_NA_VALUE, null);
context.setAllowUnmappedFields(false);
MappedFieldType fieldType = new TextFieldMapper.TextFieldType();
MappedFieldType result = context.failIfFieldMappingNotFound("name", fieldType);
Expand All @@ -91,30 +77,16 @@ public void testFailIfFieldMappingNotFound() {
}

public void testClusterAlias() throws IOException {
IndexMetaData.Builder indexMetadataBuilder = new IndexMetaData.Builder("index");
indexMetadataBuilder.settings(Settings.builder().put("index.version.created", Version.CURRENT)
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 1)
);
IndexMetaData indexMetaData = indexMetadataBuilder.build();
IndexSettings indexSettings = new IndexSettings(indexMetaData, Settings.EMPTY);
MapperService mapperService = mock(MapperService.class);
when(mapperService.getIndexSettings()).thenReturn(indexSettings);
when(mapperService.index()).thenReturn(indexMetaData.getIndex());
final long nowInMillis = randomNonNegativeLong();
final String clusterAlias = randomBoolean() ? null : "remote_cluster";
QueryShardContext context = createQueryShardContext(IndexMetaData.INDEX_UUID_NA_VALUE, clusterAlias);

Mapper.BuilderContext ctx = new Mapper.BuilderContext(indexSettings.getSettings(), new ContentPath());

Mapper.BuilderContext ctx = new Mapper.BuilderContext(context.getIndexSettings().getSettings(), new ContentPath());
IndexFieldMapper mapper = new IndexFieldMapper.Builder(null).build(ctx);
final String clusterAlias = randomBoolean() ? null : "remote_cluster";
QueryShardContext context = new QueryShardContext(
0, indexSettings, null, (mappedFieldType, indexname) ->
mappedFieldType.fielddataBuilder(indexname).build(indexSettings, mappedFieldType, null, null, mapperService)
, mapperService, null, null, xContentRegistry(), writableRegistry(), null, null,
() -> nowInMillis, clusterAlias);

IndexFieldData<?> forField = context.getForField(mapper.fieldType());
String expected = clusterAlias == null ? indexMetaData.getIndex().getName()
: clusterAlias + ":" + indexMetaData.getIndex().getName();
String expected = clusterAlias == null ? context.getIndexSettings().getIndexMetaData().getIndex().getName()
: clusterAlias + ":" + context.getIndexSettings().getIndex().getName();
assertEquals(expected, ((AbstractAtomicOrdinalsFieldData)forField.load(null)).getOrdinalsValues().lookupOrd(0).utf8ToString());
Query query = mapper.fieldType().termQuery("index", context);
if (clusterAlias == null) {
Expand All @@ -133,4 +105,32 @@ public void testClusterAlias() throws IOException {
assertThat(query, Matchers.instanceOf(MatchNoDocsQuery.class));
}

public void testGetFullyQualifiedIndex() {
String clusterAlias = randomAlphaOfLengthBetween(5, 10);
String indexUuid = randomAlphaOfLengthBetween(3, 10);
QueryShardContext shardContext = createQueryShardContext(indexUuid, clusterAlias);
assertThat(shardContext.getFullyQualifiedIndex().getName(), equalTo(clusterAlias + ":index"));
assertThat(shardContext.getFullyQualifiedIndex().getUUID(), equalTo(indexUuid));
}

public static QueryShardContext createQueryShardContext(String indexUuid, String clusterAlias) {
IndexMetaData.Builder indexMetadataBuilder = new IndexMetaData.Builder("index");
indexMetadataBuilder.settings(Settings.builder().put("index.version.created", Version.CURRENT)
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 1)
.put(IndexMetaData.SETTING_INDEX_UUID, indexUuid)
);
IndexMetaData indexMetaData = indexMetadataBuilder.build();
IndexSettings indexSettings = new IndexSettings(indexMetaData, Settings.EMPTY);
MapperService mapperService = mock(MapperService.class);
when(mapperService.getIndexSettings()).thenReturn(indexSettings);
when(mapperService.index()).thenReturn(indexMetaData.getIndex());
final long nowInMillis = randomNonNegativeLong();

return new QueryShardContext(
0, indexSettings, null, (mappedFieldType, idxName) ->
mappedFieldType.fielddataBuilder(idxName).build(indexSettings, mappedFieldType, null, null, null)
, mapperService, null, null, NamedXContentRegistry.EMPTY, new NamedWriteableRegistry(Collections.emptyList()), null, null,
() -> nowInMillis, clusterAlias);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.index.query;

import org.elasticsearch.index.Index;
import org.elasticsearch.test.ESTestCase;

import static org.hamcrest.CoreMatchers.equalTo;

public class QueryShardExceptionTests extends ESTestCase {

public void testCreateFromQueryShardContext() {
String indexUuid = randomAlphaOfLengthBetween(5, 10);
String clusterAlias = randomAlphaOfLengthBetween(5, 10);
QueryShardContext queryShardContext = QueryShardContextTests.createQueryShardContext(indexUuid, clusterAlias);
{
QueryShardException queryShardException = new QueryShardException(queryShardContext, "error");
assertThat(queryShardException.getIndex().getName(), equalTo(clusterAlias + ":index"));
assertThat(queryShardException.getIndex().getUUID(), equalTo(indexUuid));
}
{
QueryShardException queryShardException = new QueryShardException(queryShardContext, "error", new IllegalArgumentException());
assertThat(queryShardException.getIndex().getName(), equalTo(clusterAlias + ":index"));
assertThat(queryShardException.getIndex().getUUID(), equalTo(indexUuid));
}
}

public void testCreateFromIndex() {
String indexUuid = randomAlphaOfLengthBetween(5, 10);
String indexName = randomAlphaOfLengthBetween(5, 10);
Index index = new Index(indexName, indexUuid);
QueryShardException queryShardException = new QueryShardException(index, "error", new IllegalArgumentException());
assertThat(queryShardException.getIndex().getName(), equalTo(indexName));
assertThat(queryShardException.getIndex().getUUID(), equalTo(indexUuid));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import java.util.HashMap;
import java.util.Map;

import static org.elasticsearch.test.AbstractBuilderTestCase.STRING_ALIAS_FIELD_NAME;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;

Expand Down Expand Up @@ -143,7 +142,7 @@ public void testWithMetaDataField() throws IOException {

public void testIndexWildcard() throws IOException {
QueryShardContext context = createShardContext();
String index = context.getFullyQualifiedIndexName();
String index = context.getFullyQualifiedIndex().getName();

Query query = new WildcardQueryBuilder("_index", index).doToQuery(context);
assertThat(query instanceof MatchAllDocsQuery, equalTo(true));
Expand Down

0 comments on commit c474c74

Please sign in to comment.