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.

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

add support for write index resolution when creating/updating documents #31520

Merged
merged 30 commits into from
Jul 19, 2018
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
9e86921
add support for write index resolution when creating/updating documents
talevy Jun 20, 2018
df8fabd
refactor to use new resolveWriteIndex flag instead of IndicesOptions
talevy Jun 25, 2018
7c75916
Merge branch 'master' into indices-options-write-index
talevy Jun 26, 2018
113ebc5
comment out test question
talevy Jun 26, 2018
fb7c168
whitespace cleanup
talevy Jun 26, 2018
c1d2d4e
Merge branch 'master' into indices-options-write-index
talevy Jun 27, 2018
b585835
cleanup and return existing alias routing resolution
talevy Jun 27, 2018
42893bf
Merge branch 'master' into indices-options-write-index
talevy Jun 27, 2018
54d0a3d
fix typo
talevy Jun 27, 2018
b5e5e81
add successful getWriteIndex test
talevy Jun 27, 2018
d4a5ebd
simplify metadata test
talevy Jun 27, 2018
2f8a004
ease up index routing resolution.
talevy Jun 27, 2018
9238cb5
fix IT test
talevy Jun 28, 2018
8ae0967
Merge branch 'master' into indices-options-write-index
talevy Jun 28, 2018
e8cf8c8
add security test for is_write_index resolution
talevy Jun 29, 2018
2f2af95
Merge branch 'master' into indices-options-write-index
talevy Jun 29, 2018
3df26bd
Merge branch 'master' into indices-options-write-index
talevy Jul 9, 2018
5d47213
Merge branch 'master' into indices-options-write-index
talevy Jul 10, 2018
00eb394
respond to comments
talevy Jul 10, 2018
7c753db
add tests for different index expressions and indices options
talevy Jul 10, 2018
f1032e4
fix checkstyle
talevy Jul 10, 2018
347149e
fix metadata index routing tests
talevy Jul 10, 2018
e58a3fb
ignore routing on aliases pointing to multiple indices in write opera…
talevy Jul 10, 2018
222ae13
Merge branch 'master' into indices-options-write-index
talevy Jul 15, 2018
ae372f3
update routing to evaluate to write index's alias metadata for write …
talevy Jul 16, 2018
b54efe4
Merge branch 'master' into indices-options-write-index
talevy Jul 17, 2018
cda911f
add more integration tests and small cleanup
talevy Jul 17, 2018
eb097c2
fix metadata randomness
talevy Jul 17, 2018
cc969d1
cleanup bulk write routing IT test and remove Unit tests for this
talevy Jul 18, 2018
284d3ee
add randomization to bulkintegrationIT test
talevy Jul 19, 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
Original file line number Diff line number Diff line change
Expand Up @@ -295,7 +295,7 @@ protected void doRun() throws Exception {
TransportUpdateAction.resolveAndValidateRouting(metaData, concreteIndex.getName(), (UpdateRequest) docWriteRequest);
break;
case DELETE:
docWriteRequest.routing(metaData.resolveIndexRouting(docWriteRequest.routing(), docWriteRequest.index()));
docWriteRequest.routing(metaData.resolveWriteIndexRouting(docWriteRequest.routing(), docWriteRequest.index()));
// check if routing is required, if so, throw error if routing wasn't specified
if (docWriteRequest.routing() == null && metaData.routingRequired(concreteIndex.getName(), docWriteRequest.type())) {
throw new RoutingMissingException(concreteIndex.getName(), docWriteRequest.type(), docWriteRequest.id());
Expand Down Expand Up @@ -474,7 +474,7 @@ Index getConcreteIndex(String indexOrAlias) {
Index resolveIfAbsent(DocWriteRequest<?> request) {
Index concreteIndex = indices.get(request.index());
if (concreteIndex == null) {
concreteIndex = indexNameExpressionResolver.concreteSingleIndex(state, request);
concreteIndex = indexNameExpressionResolver.concreteWriteIndex(state, request);
indices.put(request.index(), concreteIndex);
}
return concreteIndex;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ public void process(Version indexCreatedVersion, @Nullable MappingMetaData mappi

/* resolve the routing if needed */
public void resolveRouting(MetaData metaData) {
routing(metaData.resolveIndexRouting(routing, index));
routing(metaData.resolveWriteIndexRouting(routing, index));
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ protected void resolveRequest(ClusterState state, UpdateRequest request) {
}

public static void resolveAndValidateRouting(MetaData metaData, String concreteIndex, UpdateRequest request) {
request.routing((metaData.resolveIndexRouting(request.routing(), request.index())));
request.routing((metaData.resolveWriteIndexRouting(request.routing(), request.index())));
// Fail fast on the node that received the request, rather than failing when translating on the index or delete request.
if (request.routing() == null && metaData.routingRequired(concreteIndex, request.type())) {
throw new RoutingMissingException(concreteIndex, request.type(), request.id());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
Expand Down Expand Up @@ -103,7 +102,7 @@ public String[] concreteIndexNames(ClusterState state, IndicesOptions options, S
return concreteIndexNames(context, indexExpressions);
}

/**
/**
* Translates the provided index expression into actual concrete indices, properly deduplicated.
*
* @param state the cluster state containing all the data to resolve to expressions to concrete indices
Expand All @@ -117,7 +116,7 @@ public String[] concreteIndexNames(ClusterState state, IndicesOptions options, S
* indices options in the context don't allow such a case.
*/
public Index[] concreteIndices(ClusterState state, IndicesOptions options, String... indexExpressions) {
Context context = new Context(state, options);
Context context = new Context(state, options, false, false);
return concreteIndices(context, indexExpressions);
}

Expand Down Expand Up @@ -193,30 +192,40 @@ Index[] concreteIndices(Context context, String... indexExpressions) {
}
}

Collection<IndexMetaData> resolvedIndices = aliasOrIndex.getIndices();
if (resolvedIndices.size() > 1 && !options.allowAliasesToMultipleIndices()) {
String[] indexNames = new String[resolvedIndices.size()];
int i = 0;
for (IndexMetaData indexMetaData : resolvedIndices) {
indexNames[i++] = indexMetaData.getIndex().getName();
if (aliasOrIndex.isAlias() && context.isResolveToWriteIndex()) {
AliasOrIndex.Alias alias = (AliasOrIndex.Alias) aliasOrIndex;
IndexMetaData writeIndex = alias.getWriteIndex();
if (writeIndex == null) {
throw new IllegalArgumentException("no write index is defined for alias [" + alias.getAliasName() + "]." +
" The write index may be explicitly disabled using is_write_index=false or the alias points to multiple" +
" indices without one being designated as a write index");
}
throw new IllegalArgumentException("Alias [" + expression + "] has more than one indices associated with it [" +
concreteIndices.add(writeIndex.getIndex());
} else {
if (aliasOrIndex.getIndices().size() > 1 && !options.allowAliasesToMultipleIndices()) {
String[] indexNames = new String[aliasOrIndex.getIndices().size()];
int i = 0;
for (IndexMetaData indexMetaData : aliasOrIndex.getIndices()) {
indexNames[i++] = indexMetaData.getIndex().getName();
}
throw new IllegalArgumentException("Alias [" + expression + "] has more than one indices associated with it [" +
Arrays.toString(indexNames) + "], can't execute a single index op");
}
}

for (IndexMetaData index : resolvedIndices) {
if (index.getState() == IndexMetaData.State.CLOSE) {
if (failClosed) {
throw new IndexClosedException(index.getIndex());
} else {
if (options.forbidClosedIndices() == false) {
concreteIndices.add(index.getIndex());
for (IndexMetaData index : aliasOrIndex.getIndices()) {
if (index.getState() == IndexMetaData.State.CLOSE) {
if (failClosed) {
throw new IndexClosedException(index.getIndex());
} else {
if (options.forbidClosedIndices() == false) {
concreteIndices.add(index.getIndex());
}
}
} else if (index.getState() == IndexMetaData.State.OPEN) {
concreteIndices.add(index.getIndex());
} else {
throw new IllegalStateException("index state [" + index.getState() + "] not supported");
}
} else if (index.getState() == IndexMetaData.State.OPEN) {
concreteIndices.add(index.getIndex());
} else {
throw new IllegalStateException("index state [" + index.getState() + "] not supported");
}
}
}
Expand Down Expand Up @@ -255,6 +264,28 @@ public Index concreteSingleIndex(ClusterState state, IndicesRequest request) {
return indices[0];
}

/**
* Utility method that allows to resolve an index expression to its corresponding single write index.
*
* @param state the cluster state containing all the data to resolve to expression to a concrete index
* @param request The request that defines how the an alias or an index need to be resolved to a concrete index
* and the expression that can be resolved to an alias or an index name.
* @throws IllegalArgumentException if the index resolution does not lead to an index, or leads to more than one index
* @return the write index obtained as a result of the index resolution
*/
public Index concreteWriteIndex(ClusterState state, IndicesRequest request) {
if (request.indices() == null || (request.indices() != null && request.indices().length != 1)) {
throw new IllegalArgumentException("indices request must specify a single index expression");
}
Context context = new Context(state, request.indicesOptions(), false, true);
Index[] indices = concreteIndices(context, request.indices()[0]);
if (indices.length != 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

throw new IllegalArgumentException("The index expression [" + request.indices()[0] +
"] and options provided did not point to a single write-index");
}
return indices[0];
}

/**
* @return whether the specified alias or index exists. If the alias or index contains datemath then that is resolved too.
*/
Expand Down Expand Up @@ -292,7 +323,7 @@ public String[] indexAliases(ClusterState state, String index, Predicate<AliasMe
String... expressions) {
// expand the aliases wildcard
List<String> resolvedExpressions = expressions != null ? Arrays.asList(expressions) : Collections.emptyList();
Context context = new Context(state, IndicesOptions.lenientExpandOpen(), true);
Context context = new Context(state, IndicesOptions.lenientExpandOpen(), true, false);
for (ExpressionResolver expressionResolver : expressionResolvers) {
resolvedExpressions = expressionResolver.resolve(context, resolvedExpressions);
}
Expand Down Expand Up @@ -512,24 +543,26 @@ static final class Context {
private final IndicesOptions options;
private final long startTime;
private final boolean preserveAliases;
private final boolean resolveToWriteIndex;

Context(ClusterState state, IndicesOptions options) {
this(state, options, System.currentTimeMillis());
}

Context(ClusterState state, IndicesOptions options, boolean preserveAliases) {
this(state, options, System.currentTimeMillis(), preserveAliases);
Context(ClusterState state, IndicesOptions options, boolean preserveAliases, boolean resolveToWriteIndex) {
this(state, options, System.currentTimeMillis(), preserveAliases, resolveToWriteIndex);
}

Context(ClusterState state, IndicesOptions options, long startTime) {
this(state, options, startTime, false);
this(state, options, startTime, false, false);
}

Context(ClusterState state, IndicesOptions options, long startTime, boolean preserveAliases) {
Context(ClusterState state, IndicesOptions options, long startTime, boolean preserveAliases, boolean resolveToWriteIndex) {
this.state = state;
this.options = options;
this.startTime = startTime;
this.preserveAliases = preserveAliases;
this.resolveToWriteIndex = resolveToWriteIndex;
}

public ClusterState getState() {
Expand All @@ -552,6 +585,14 @@ public long getStartTime() {
boolean isPreserveAliases() {
return preserveAliases;
}

/**
* This is used to require that aliases resolve to their write-index. It is currently not used in conjunction
* with <code>preserveAliases</code>.
*/
boolean isResolveToWriteIndex() {
return resolveToWriteIndex;
}
}

private interface ExpressionResolver {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,42 @@ public String[] getConcreteAllClosedIndices() {
return allClosedIndices;
}

/**
* Returns indexing routing for the given <code>aliasOrIndex</code>. Resolves routing from the alias metadata used
* in the write index.
*/
public String resolveWriteIndexRouting(@Nullable String routing, String aliasOrIndex) {
if (aliasOrIndex == null) {
return routing;
}

AliasOrIndex result = getAliasAndIndexLookup().get(aliasOrIndex);
if (result == null || result.isAlias() == false) {
return routing;
}
AliasOrIndex.Alias alias = (AliasOrIndex.Alias) result;
IndexMetaData writeIndex = alias.getWriteIndex();
if (writeIndex == null) {
throw new IllegalArgumentException("alias [" + aliasOrIndex + "] does not have a write index");
}
AliasMetaData aliasMd = writeIndex.getAliases().get(alias.getAliasName());
if (aliasMd.indexRouting() != null) {
if (aliasMd.indexRouting().indexOf(',') != -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should validate that when people set is_write_index to true as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that to do this validation justice, it will have to be looked at as something that can be invalid for both read and write. Since this check is similar in both. For this reason, I would feel better if that stricter validation be looked at outside this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. I agree it's not straight forward. Let's have another PR (I tend to say that multiple routing values changes the default for is_write_index and disallows setting it)

throw new IllegalArgumentException("index/alias [" + aliasOrIndex + "] provided with routing value ["
+ aliasMd.getIndexRouting() + "] that resolved to several routing values, rejecting operation");
}
if (routing != null) {
if (!routing.equals(aliasMd.indexRouting())) {
throw new IllegalArgumentException("Alias [" + aliasOrIndex + "] has index routing associated with it ["
+ aliasMd.indexRouting() + "], and was provided with routing value [" + routing + "], rejecting operation");
}
}
// Alias routing overrides the parent routing (if any).
return aliasMd.indexRouting();
}
return routing;
}

/**
* Returns indexing routing for the given index.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,21 @@

package org.elasticsearch.action.bulk;

import org.elasticsearch.action.admin.indices.alias.Alias;
import org.elasticsearch.action.admin.indices.mapping.get.GetMappingsResponse;
import org.elasticsearch.action.get.GetRequestBuilder;
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.ESIntegTestCase;

import java.nio.charset.StandardCharsets;
import java.util.Collections;
import java.util.Map;

import static org.elasticsearch.test.StreamsUtils.copyToStringFromClasspath;
import static org.hamcrest.Matchers.equalTo;

public class BulkIntegrationIT extends ESIntegTestCase {
public void testBulkIndexCreatesMapping() throws Exception {
Expand All @@ -40,4 +48,51 @@ public void testBulkIndexCreatesMapping() throws Exception {
assertTrue(mappingsResponse.getMappings().get("logstash-2014.03.30").containsKey("logs"));
});
}

/**
* This tests that the {@link TransportBulkAction} evaluates alias routing values correctly when dealing with
* an alias pointing to multiple indices, while a write index exits.
*/
public void testBulkWithWriteIndexAndRouting() {
Map<String, Integer> twoShardsSettings = Collections.singletonMap(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 2);
client().admin().indices().prepareCreate("index1")
.addAlias(new Alias("alias1").indexRouting("0")).setSettings(twoShardsSettings).get();
client().admin().indices().prepareCreate("index2")
.addAlias(new Alias("alias1").indexRouting("0").writeIndex(randomFrom(false, null)))
.setSettings(twoShardsSettings).get();
client().admin().indices().prepareCreate("index3")
.addAlias(new Alias("alias1").indexRouting("1").writeIndex(true)).setSettings(twoShardsSettings).get();

IndexRequest indexRequest = new IndexRequest("index3", "type", "id");
boolean indexWithRouting = randomBoolean();
if (indexWithRouting) {
indexRequest.routing("0");
}
indexRequest.source(Collections.singletonMap("foo", "bar"));
IndexRequest indexRequestWithAlias = new IndexRequest("alias1", "type", "id");
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use different ids for the different indices? I find this super confusing to reason about. Maybe also add the routing value you expect to be used to the id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is also OK to remove the first index request entirely. I just wanted to it to demonstrate that the two calls do not interact with the same document, even though they have the same index/type/id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, I remember the purpose of this other index request. It is to verify the other branches of the routing resolution. The reason I think it is OK to remove is that this IT test is meant to test the connection to this method, not all its branches. The Unit tests do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

randomly set the routing to "1"?

indexRequestWithAlias.source(Collections.singletonMap("foo", "baz"));
BulkResponse bulkResponse = client().prepareBulk()
.add(indexRequest).add(indexRequestWithAlias).get();
assertThat(bulkResponse.getItems()[0].getResponse().getIndex(), equalTo("index3"));
assertThat(bulkResponse.getItems()[0].getResponse().getShardId().getId(), equalTo(1));
assertThat(bulkResponse.getItems()[0].getResponse().getVersion(), equalTo(1L));
assertThat(bulkResponse.getItems()[0].getResponse().status(), equalTo(RestStatus.CREATED));
assertThat(bulkResponse.getItems()[1].getResponse().getIndex(), equalTo("index3"));
assertThat(bulkResponse.getItems()[1].getResponse().getShardId().getId(), equalTo(0));
assertThat(bulkResponse.getItems()[1].getResponse().getVersion(), equalTo(1L));
assertThat(bulkResponse.getItems()[1].getResponse().status(), equalTo(RestStatus.CREATED));
GetRequestBuilder getBuilder = client().prepareGet("index3", "type", "id");
if (indexWithRouting) {
getBuilder.setRouting("0");
}
assertThat(getBuilder.get().getSource().get("foo"), equalTo("bar"));
assertThat(client().prepareGet("index3", "type", "id").setRouting("1").get().getSource().get("foo"), equalTo("baz"));

client().prepareBulk().add(client().prepareUpdate("alias1", "type", "id").setDoc("foo", "updated")).get();
Copy link
Contributor

Choose a reason for hiding this comment

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

we should check errors here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add. didn't think this was necessary since there are get-action calls verifying success of this operation

assertThat(getBuilder.get().getSource().get("foo"), equalTo("bar"));
assertThat(client().prepareGet("index3", "type", "id").setRouting("1").get().getSource().get("foo"), equalTo("updated"));
client().prepareBulk().add(client().prepareDelete("alias1", "type", "id")).get();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what we're testing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TransportBulkAction resolves routing for delete requests just as it does for index and update. This delete request is here to verify this

assertThat(getBuilder.get().getSource().get("foo"), equalTo("bar"));
assertFalse(client().prepareGet("index3", "type", "id").setRouting("1").get().isExists());
}
}
Loading