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

Made single shards APIs fail if routing is configured to be required in the mapping #4523

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.action;

import org.elasticsearch.ElasticSearchException;
import org.elasticsearch.rest.RestStatus;

/**
*
Expand Down Expand Up @@ -50,4 +51,9 @@ public String type() {
public String id() {
return id;
}

@Override
public RestStatus status() {
return RestStatus.BAD_REQUEST;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.lucene.search.Explanation;
import org.elasticsearch.ElasticSearchException;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.RoutingMissingException;
import org.elasticsearch.action.support.single.shard.TransportShardSingleOperationAction;
import org.elasticsearch.cache.recycler.CacheRecycler;
import org.elasticsearch.cluster.ClusterService;
Expand Down Expand Up @@ -95,6 +96,11 @@ protected void resolveRequest(ClusterState state, ExplainRequest request) {
String concreteIndex = state.metaData().concreteIndex(request.index());
request.filteringAlias(state.metaData().filteringAliases(concreteIndex, request.index()));
request.index(state.metaData().concreteIndex(request.index()));

// Fail fast on the node that received the request.
if (request.routing() == null && state.getMetaData().routingRequired(request.index(), request.type())) {
throw new RoutingMissingException(request.index(), request.type(), request.id());
}
}

protected ExplainResponse shardOperation(ExplainRequest request, int shardId) throws ElasticSearchException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.action.get;

import org.elasticsearch.ElasticSearchException;
import org.elasticsearch.action.RoutingMissingException;
import org.elasticsearch.action.support.single.shard.TransportShardSingleOperationAction;
import org.elasticsearch.cluster.ClusterService;
import org.elasticsearch.cluster.ClusterState;
Expand Down Expand Up @@ -89,6 +90,11 @@ protected void resolveRequest(ClusterState state, GetRequest request) {
// update the routing (request#index here is possibly an alias)
request.routing(state.metaData().resolveIndexRouting(request.routing(), request.index()));
request.index(state.metaData().concreteIndex(request.index()));

// Fail fast on the node that received the request.
if (request.routing() == null && state.getMetaData().routingRequired(request.index(), request.type())) {
throw new RoutingMissingException(request.index(), request.type(), request.id());
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ protected void doExecute(final MultiGetRequest request, final ActionListener<Mul
responses.set(i, new MultiGetItemResponse(null, new MultiGetResponse.Failure(item.index(), item.type(), item.id(), "[" + item.index() + "] missing")));
continue;
}
if (item.routing() == null && clusterState.getMetaData().routingRequired(item.index(), item.type())) {
responses.set(i, new MultiGetItemResponse(null, new MultiGetResponse.Failure(item.index(), item.type(), item.id(), "routing is required, but hasn't been specified")));
continue;
}

item.routing(clusterState.metaData().resolveIndexRouting(item.routing(), item.index()));
item.index(clusterState.metaData().concreteIndex(item.index()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,14 @@ public EnumSet<Flag> getFlags() {
return flagsEnum;
}

/**
* Sets the type of document to get the term vector for.
*/
public TermVectorRequest type(String type) {
this.type = type;
return this;
}

/**
* Returns the type of document to get the term vector for.
*/
Expand All @@ -106,8 +114,9 @@ public String id() {
/**
* Sets the id of document the term vector is requested for.
*/
public void id(String id) {
public TermVectorRequest id(String id) {
this.id = id;
return this;
}

/**
Expand All @@ -117,8 +126,9 @@ public String routing() {
return routing;
}

public void routing(String routing) {
public TermVectorRequest routing(String routing) {
this.routing = routing;
return this;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ protected void doExecute(final MultiTermVectorsRequest request, final ActionList
termVectorRequest.type(), termVectorRequest.id(), "[" + termVectorRequest.index() + "] missing")));
continue;
}
if (termVectorRequest.routing() == null && clusterState.getMetaData().routingRequired(termVectorRequest.index(), termVectorRequest.type())) {
responses.set(i, new MultiTermVectorsItemResponse(null, new MultiTermVectorsResponse.Failure(termVectorRequest.index(),
termVectorRequest.type(), termVectorRequest.id(), "routing is required, but hasn't been specified")));
continue;
}
termVectorRequest.index(clusterState.metaData().concreteIndex(termVectorRequest.index()));
ShardId shardId = clusterService
.operationRouting()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.elasticsearch.action.termvector;

import org.elasticsearch.ElasticSearchException;
import org.elasticsearch.action.RoutingMissingException;
import org.elasticsearch.action.support.single.shard.TransportShardSingleOperationAction;
import org.elasticsearch.cluster.ClusterService;
import org.elasticsearch.cluster.ClusterState;
Expand Down Expand Up @@ -80,6 +81,11 @@ protected void resolveRequest(ClusterState state, TermVectorRequest request) {
// update the routing (request#index here is possibly an alias)
request.routing(state.metaData().resolveIndexRouting(request.routing(), request.index()));
request.index(state.metaData().concreteIndex(request.index()));

// Fail fast on the node that received the request.
if (request.routing() == null && state.getMetaData().routingRequired(request.index(), request.type())) {
throw new RoutingMissingException(request.index(), request.type(), request.id());
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.ElasticSearchIllegalStateException;
import org.elasticsearch.ExceptionsHelper;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.RoutingMissingException;
import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
import org.elasticsearch.action.admin.indices.create.CreateIndexResponse;
import org.elasticsearch.action.admin.indices.create.TransportCreateIndexAction;
Expand Down Expand Up @@ -121,6 +122,11 @@ protected boolean resolveRequest(ClusterState state, UpdateRequest request, Acti
String aliasOrIndex = request.index();
request.routing((metaData.resolveIndexRouting(request.routing(), aliasOrIndex)));
request.index(metaData.concreteIndex(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 && state.getMetaData().routingRequired(request.index(), request.type())) {
throw new RoutingMissingException(request.index(), request.type(), request.id());
}
return true;
}

Expand Down
16 changes: 16 additions & 0 deletions src/main/java/org/elasticsearch/cluster/metadata/MetaData.java
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,22 @@ public boolean isPatternMatchingAllIndices(String[] indicesOrAliases, String[] c
return false;
}

/**
* @param concreteIndex The concrete index to check if routing is required
* @param type The type to check if routing is required
* @return Whether routing is required according to the mapping for the specified index and type
*/
public boolean routingRequired(String concreteIndex, String type) {
IndexMetaData indexMetaData = indices.get(concreteIndex);
if (indexMetaData != null) {
MappingMetaData mappingMetaData = indexMetaData.getMappings().get(type);
if (mappingMetaData != null) {
return mappingMetaData.routing().required();
}
}
return false;
}

@Override
public UnmodifiableIterator<IndexMetaData> iterator() {
return indices.valuesIt();
Expand Down
5 changes: 3 additions & 2 deletions src/test/java/org/elasticsearch/mget/SimpleMgetTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,9 @@ public void testThatParentPerDocumentIsSupported() throws Exception {
assertThat(mgetResponse.getResponses()[0].isFailed(), is(false));
assertThat(mgetResponse.getResponses()[0].getResponse().isExists(), is(true));

assertThat(mgetResponse.getResponses()[1].isFailed(), is(false));
assertThat(mgetResponse.getResponses()[1].getResponse().isExists(), is(false));
assertThat(mgetResponse.getResponses()[1].isFailed(), is(true));
assertThat(mgetResponse.getResponses()[1].getResponse(), nullValue());
assertThat(mgetResponse.getResponses()[1].getFailure().getMessage(), equalTo("routing is required, but hasn't been specified"));
}

@SuppressWarnings("unchecked")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,12 @@ public void testRequiredRoutingMappingWithAlias() throws Exception {
logger.info("--> deleting with no routing, should broadcast the delete since _routing is required");
client().prepareDelete("test", "type1", "1").setRefresh(true).execute().actionGet();
for (int i = 0; i < 5; i++) {
assertThat(client().prepareGet("test", "type1", "1").execute().actionGet().isExists(), equalTo(false));
try {
client().prepareGet("test", "type1", "1").get();
assert false;
} catch (RoutingMissingException e) {
assertThat(e.getMessage(), equalTo("routing is required for [test]/[type1]/[1]"));
}
assertThat(client().prepareGet("test", "type1", "1").setRouting("0").execute().actionGet().isExists(), equalTo(false));
}

Expand Down