SOLR-4574: The Collections API will silently return success on an unknown ACTION parameter.

SOLR-4576: Collections API validation errors should cause an exception on clients and otherwise act as validation errors with the Core Admin API.
SOLR-4577: The collections API should return responses (success or failure) for each node it attempts to work with. 

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1456683 13f79535-47bb-0310-9956-ffa450edef68
This commit is contained in:
Mark Robert Miller 2013-03-14 21:01:00 +00:00
parent 2c64eb9019
commit a9fd4c7a3f
9 changed files with 266 additions and 86 deletions

View File

@ -93,6 +93,16 @@ Bug Fixes
close it in SolrCloud mode when a core with the same name already exists.
(Mark Miller)
* SOLR-4574: The Collections API will silently return success on an unknown
ACTION parameter. (Mark Miller)
* SOLR-4576: Collections API validation errors should cause an exception on
clients and otherwise act as validation errors with the Core Admin API.
(Mark Miller)
* SOLR-4577: The collections API should return responses (success or failure)
for each node it attempts to work with. (Mark Miller)
Other Changes
----------------------

View File

@ -41,6 +41,7 @@ import org.apache.solr.common.params.CoreAdminParams;
import org.apache.solr.common.params.CoreAdminParams.CoreAdminAction;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.common.util.SimpleOrderedMap;
import org.apache.solr.common.util.StrUtils;
import org.apache.solr.handler.component.ShardHandler;
import org.apache.solr.handler.component.ShardRequest;
@ -156,22 +157,22 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
NamedList results = new NamedList();
try {
if (CREATECOLLECTION.equals(operation)) {
createCollection(zkStateReader.getClusterState(), message);
createCollection(zkStateReader.getClusterState(), message, results);
} else if (DELETECOLLECTION.equals(operation)) {
ModifiableSolrParams params = new ModifiableSolrParams();
params.set(CoreAdminParams.ACTION, CoreAdminAction.UNLOAD.toString());
params.set(CoreAdminParams.DELETE_INSTANCE_DIR, true);
collectionCmd(zkStateReader.getClusterState(), message, params);
collectionCmd(zkStateReader.getClusterState(), message, params, results);
} else if (RELOADCOLLECTION.equals(operation)) {
ModifiableSolrParams params = new ModifiableSolrParams();
params.set(CoreAdminParams.ACTION, CoreAdminAction.RELOAD.toString());
collectionCmd(zkStateReader.getClusterState(), message, params);
collectionCmd(zkStateReader.getClusterState(), message, params, results);
} else if (CREATEALIAS.equals(operation)) {
createAlias(zkStateReader.getAliases(), message);
} else if (DELETEALIAS.equals(operation)) {
deleteAlias(zkStateReader.getAliases(), message);
} else {
throw new SolrException(ErrorCode.BAD_REQUEST, "Unknow the operation:"
throw new SolrException(ErrorCode.BAD_REQUEST, "Unknown operation:"
+ operation);
}
int failed = 0;
@ -194,6 +195,10 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
SolrException.log(log, "Collection " + operation + " of " + operation
+ " failed", ex);
results.add("Operation " + operation + " caused exception:", ex);
SimpleOrderedMap nl = new SimpleOrderedMap();
nl.add("msg", ex.getMessage());
nl.add("rspCode", ex instanceof SolrException ? ((SolrException)ex).code() : -1);
results.add("exception", nl);
} finally {
return new OverseerSolrResponse(results);
}
@ -300,11 +305,10 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
}
private boolean createCollection(ClusterState clusterState, ZkNodeProps message) {
private void createCollection(ClusterState clusterState, ZkNodeProps message, NamedList results) {
String collectionName = message.getStr("name");
if (clusterState.getCollections().contains(collectionName)) {
SolrException.log(log, "collection already exists: " + collectionName);
return false;
throw new SolrException(ErrorCode.BAD_REQUEST, "collection already exists: " + collectionName);
}
try {
@ -319,12 +323,11 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
if (repFactor <= 0) {
SolrException.log(log, REPLICATION_FACTOR + " must be > 0");
return false;
throw new SolrException(ErrorCode.BAD_REQUEST, "collection already exists: " + collectionName);
}
if (numSlices < 0) {
SolrException.log(log, NUM_SLICES + " must be > 0");
return false;
throw new SolrException(ErrorCode.BAD_REQUEST, NUM_SLICES + " must be > 0");
}
String configName = message.getStr("collection.configName");
@ -343,9 +346,8 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
Collections.shuffle(nodeList);
if (nodeList.size() <= 0) {
log.error("Cannot create collection " + collectionName
+ ". No live Solr-instaces" + ((createNodeList != null)?" among Solr-instances specified in " + CREATE_NODE_SET:""));
return false;
throw new SolrException(ErrorCode.BAD_REQUEST, "Cannot create collection " + collectionName
+ ". No live Solr-instances" + ((createNodeList != null)?" among Solr-instances specified in " + CREATE_NODE_SET + ":" + createNodeSetStr:""));
}
if (repFactor > nodeList.size()) {
@ -363,7 +365,7 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
int maxShardsAllowedToCreate = maxShardsPerNode * nodeList.size();
int requestedShardsToCreate = numSlices * repFactor;
if (maxShardsAllowedToCreate < requestedShardsToCreate) {
log.error("Cannot create collection " + collectionName + ". Value of "
throw new SolrException(ErrorCode.BAD_REQUEST, "Cannot create collection " + collectionName + ". Value of "
+ MAX_SHARDS_PER_NODE + " is " + maxShardsPerNode
+ ", and the number of live nodes is " + nodeList.size()
+ ". This allows a maximum of " + maxShardsAllowedToCreate
@ -371,7 +373,6 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
+ " and value of " + REPLICATION_FACTOR + " is " + repFactor
+ ". This requires " + requestedShardsToCreate
+ " shards to be created (higher than the allowed number)");
return false;
}
for (int i = 1; i <= numSlices; i++) {
@ -394,6 +395,7 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
params.set(ZkStateReader.NUM_SHARDS_PROP, numSlices);
ShardRequest sreq = new ShardRequest();
sreq.nodeName = nodeName;
params.set("qt", adminPath);
sreq.purpose = 1;
String replica = zkStateReader.getZkClient()
@ -408,36 +410,25 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
}
}
int failed = 0;
ShardResponse srsp;
do {
srsp = shardHandler.takeCompletedOrError();
if (srsp != null) {
Throwable e = srsp.getException();
if (e != null) {
// should we retry?
// TODO: we should return errors to the client
// TODO: what if one fails and others succeed?
failed++;
log.error("Error talking to shard: " + srsp.getShard(), e);
}
processResponse(results, srsp);
}
} while (srsp != null);
// if all calls succeeded, return true
if (failed > 0) {
return false;
}
log.info("Successfully created all shards for collection "
log.info("Finished create command on all shards for collection: "
+ collectionName);
return true;
} catch (SolrException ex) {
throw ex;
} catch (Exception ex) {
// Expecting that the necessary logging has already been performed
return false;
throw new SolrException(ErrorCode.SERVER_ERROR, null, ex);
}
}
private boolean collectionCmd(ClusterState clusterState, ZkNodeProps message, ModifiableSolrParams params) {
private boolean collectionCmd(ClusterState clusterState, ZkNodeProps message, ModifiableSolrParams params, NamedList results) {
log.info("Executing Collection Cmd : " + params);
String collectionName = message.getStr("name");
@ -463,7 +454,7 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
String replica = node.getStr(ZkStateReader.BASE_URL_PROP);
ShardRequest sreq = new ShardRequest();
sreq.nodeName = node.getStr(ZkStateReader.NODE_NAME_PROP);
// yes, they must use same admin handler path everywhere...
cloneParams.set("qt", adminPath);
sreq.purpose = 1;
@ -484,14 +475,7 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
do {
srsp = shardHandler.takeCompletedOrError();
if (srsp != null) {
Throwable e = srsp.getException();
if (e != null) {
// should we retry?
// TODO: we should return errors to the client
// TODO: what if one fails and others succeed?
failed++;
log.error("Error talking to shard: " + srsp.getShard(), e);
}
processResponse(results, srsp);
}
} while (srsp != null);
@ -503,6 +487,31 @@ public class OverseerCollectionProcessor implements Runnable, ClosableThread {
return true;
}
private void processResponse(NamedList results, ShardResponse srsp) {
Throwable e = srsp.getException();
if (e != null) {
log.error("Error from shard: " + srsp.getShard(), e);
SimpleOrderedMap failure = (SimpleOrderedMap) results.get("failure");
if (failure == null) {
failure = new SimpleOrderedMap();
results.add("failure", failure);
}
failure.add(srsp.getNodeName(), e.getClass().getName() + ":" + e.getMessage());
} else {
SimpleOrderedMap success = (SimpleOrderedMap) results.get("success");
if (success == null) {
success = new SimpleOrderedMap();
results.add("success", success);
}
success.add(srsp.getNodeName(), srsp.getSolrResponse().getResponse());
}
}
private int msgStrToInt(ZkNodeProps message, String key, Integer def)
throws Exception {
String str = message.getStr(key);

View File

@ -40,6 +40,7 @@ import org.apache.solr.common.params.CoreAdminParams;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.params.SolrParams;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.common.util.SimpleOrderedMap;
import org.apache.solr.core.CoreContainer;
import org.apache.solr.handler.RequestHandlerBase;
import org.apache.solr.request.SolrQueryRequest;
@ -102,35 +103,38 @@ public class CollectionsHandler extends RequestHandlerBase {
if (a != null) {
action = CollectionAction.get(a);
}
if (action != null) {
switch (action) {
case CREATE: {
this.handleCreateAction(req, rsp);
break;
}
case DELETE: {
this.handleDeleteAction(req, rsp);
break;
}
case RELOAD: {
this.handleReloadAction(req, rsp);
break;
}
case SYNCSHARD: {
this.handleSyncShardAction(req, rsp);
break;
}
case CREATEALIAS: {
this.handleCreateAliasAction(req, rsp);
break;
}
case DELETEALIAS: {
this.handleDeleteAliasAction(req, rsp);
break;
}
default: {
throw new RuntimeException("Unknown action: " + action);
}
if (action == null) {
throw new SolrException(ErrorCode.BAD_REQUEST, "Unknown action: " + a);
}
switch (action) {
case CREATE: {
this.handleCreateAction(req, rsp);
break;
}
case DELETE: {
this.handleDeleteAction(req, rsp);
break;
}
case RELOAD: {
this.handleReloadAction(req, rsp);
break;
}
case SYNCSHARD: {
this.handleSyncShardAction(req, rsp);
break;
}
case CREATEALIAS: {
this.handleCreateAliasAction(req, rsp);
break;
}
case DELETEALIAS: {
this.handleDeleteAliasAction(req, rsp);
break;
}
default: {
throw new SolrException(ErrorCode.BAD_REQUEST, "Unknown action: "
+ action);
}
}
@ -148,6 +152,11 @@ public class CollectionsHandler extends RequestHandlerBase {
if (event.getBytes() != null) {
SolrResponse response = SolrResponse.deserialize(event.getBytes());
rsp.getValues().addAll(response.getResponse());
SimpleOrderedMap exp = (SimpleOrderedMap) response.getResponse().get("exception");
if (exp != null) {
Integer code = (Integer) exp.get("rspCode");
rsp.setException(new SolrException(code != null && code != -1 ? ErrorCode.getErrorCode(code) : ErrorCode.SERVER_ERROR, (String)exp.get("msg")));
}
} else {
if (System.currentTimeMillis() - time >= DEFAULT_ZK_TIMEOUT) {
throw new SolrException(ErrorCode.SERVER_ERROR, operation

View File

@ -137,6 +137,9 @@ public class HttpShardHandler extends ShardHandler {
public ShardResponse call() throws Exception {
ShardResponse srsp = new ShardResponse();
if (sreq.nodeName != null) {
srsp.setNodeName(sreq.nodeName);
}
srsp.setShardRequest(sreq);
srsp.setShard(shard);
SimpleSolrResponse ssr = new SimpleSolrResponse();

View File

@ -53,6 +53,9 @@ public class ShardRequest {
/** actual shards to send the request to, filled out by framework */
public String[] actualShards;
/** may be null */
public String nodeName;
// TODO: one could store a list of numbers to correlate where returned docs
// go in the top-level response rather than looking up by id...
// this would work well if we ever transitioned to using internal ids and

View File

@ -22,6 +22,7 @@ import org.apache.solr.common.SolrException;
public final class ShardResponse {
private ShardRequest req;
private String shard;
private String nodeName;
private String shardAddress; // the specific shard that this response was received from
private int rspCode;
private Throwable exception;
@ -56,6 +57,11 @@ public final class ShardResponse {
return shard;
}
public String getNodeName()
{
return nodeName;
}
public void setShardRequest(ShardRequest rsp)
{
this.req = rsp;
@ -81,8 +87,14 @@ public final class ShardResponse {
this.rspCode = rspCode;
}
void setNodeName(String nodeName)
{
this.nodeName = nodeName;
}
/** What was the shard address that returned this response. Example: "http://localhost:8983/solr" */
public String getShardAddress() { return this.shardAddress; }
void setShardAddress(String addr) { this.shardAddress = addr; }
}

View File

@ -17,9 +17,9 @@ package org.apache.solr.cloud;
* limitations under the License.
*/
import java.io.File;
import java.io.IOException;
import java.lang.management.ManagementFactory;
import java.net.MalformedURLException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
@ -43,12 +43,14 @@ import javax.management.ObjectName;
import org.apache.lucene.util.LuceneTestCase.Slow;
import org.apache.lucene.util._TestUtil;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.SolrQuery;
import org.apache.solr.client.solrj.SolrServerException;
import org.apache.solr.client.solrj.impl.CloudSolrServer;
import org.apache.solr.client.solrj.impl.HttpSolrServer;
import org.apache.solr.client.solrj.request.CoreAdminRequest;
import org.apache.solr.client.solrj.request.QueryRequest;
import org.apache.solr.client.solrj.request.CoreAdminRequest.Create;
import org.apache.solr.client.solrj.response.CoreAdminResponse;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.common.SolrException;
@ -63,9 +65,12 @@ import org.apache.solr.common.cloud.ZkNodeProps;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.common.params.CollectionParams.CollectionAction;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.common.util.SimpleOrderedMap;
import org.apache.solr.common.util.StrUtils;
import org.apache.solr.core.SolrCore;
import org.apache.solr.core.SolrInfoMBean.Category;
import org.apache.solr.servlet.SolrDispatchFilter;
import org.apache.solr.update.DirectUpdateHandler2;
import org.apache.solr.update.SolrCmdDistributor.Request;
import org.apache.solr.util.DefaultSolrThreadFactory;
@ -139,12 +144,115 @@ public class CollectionsAPIDistributedZkTest extends AbstractFullDistribZkTestBa
testNodesUsedByCreate();
testCollectionsAPI();
testErrorHandling();
if (DEBUG) {
super.printLayout();
}
}
private void testErrorHandling() throws Exception {
final String baseUrl = getBaseUrl((HttpSolrServer) clients.get(0));
// try a bad action
ModifiableSolrParams params = new ModifiableSolrParams();
params.set("action", "BADACTION");
String collectionName = "badactioncollection";
params.set("name", collectionName);
QueryRequest request = new QueryRequest(params);
request.setPath("/admin/collections");
boolean gotExp = false;
NamedList<Object> resp = null;
try {
resp = createNewSolrServer("", baseUrl).request(request);
} catch (SolrException e) {
gotExp = true;
}
assertTrue(gotExp);
// leave out required param name
params = new ModifiableSolrParams();
params.set("action", CollectionAction.CREATE.toString());
collectionName = "collection";
// No Name
// params.set("name", collectionName);
request = new QueryRequest(params);
request.setPath("/admin/collections");
gotExp = false;
resp = null;
try {
resp = createNewSolrServer("", baseUrl).request(request);
} catch (SolrException e) {
gotExp = true;
}
assertTrue(gotExp);
// Too many replicas
params = new ModifiableSolrParams();
params.set("action", CollectionAction.CREATE.toString());
collectionName = "collection";
params.set("name", collectionName);
params.set("numShards", 2);
params.set(OverseerCollectionProcessor.REPLICATION_FACTOR, 10);
request = new QueryRequest(params);
request.setPath("/admin/collections");
gotExp = false;
resp = null;
try {
resp = createNewSolrServer("", baseUrl).request(request);
} catch (SolrException e) {
gotExp = true;
}
assertTrue(gotExp);
// Fail on one node
// first we make a core with the core name the collections api
// will try and use - this will cause our mock fail
Create createCmd = new Create();
createCmd.setCoreName("halfcollection_shard1_replica1");
createCmd.setCollection("halfcollectionblocker");
String dataDir = SolrTestCaseJ4.dataDir.getAbsolutePath() + File.separator
+ System.currentTimeMillis() + "halfcollection" + "_3n";
createCmd.setDataDir(dataDir);
createCmd.setNumShards(1);
createNewSolrServer("", baseUrl).request(createCmd);
createCmd = new Create();
createCmd.setCoreName("halfcollection_shard1_replica1");
createCmd.setCollection("halfcollectionblocker2");
dataDir = SolrTestCaseJ4.dataDir.getAbsolutePath() + File.separator
+ System.currentTimeMillis() + "halfcollection" + "_3n";
createCmd.setDataDir(dataDir);
createCmd.setNumShards(1);
createNewSolrServer("", getBaseUrl((HttpSolrServer) clients.get(1))).request(createCmd);
params = new ModifiableSolrParams();
params.set("action", CollectionAction.CREATE.toString());
collectionName = "halfcollection";
params.set("name", collectionName);
params.set("numShards", 2);
params.set("wt", "xml");
String nn1 = ((SolrDispatchFilter) jettys.get(0).getDispatchFilter().getFilter()).getCores().getZkController().getNodeName();
String nn2 = ((SolrDispatchFilter) jettys.get(1).getDispatchFilter().getFilter()).getCores().getZkController().getNodeName();
params.set(OverseerCollectionProcessor.CREATE_NODE_SET, nn1 + "," + nn2);
request = new QueryRequest(params);
request.setPath("/admin/collections");
gotExp = false;
resp = createNewSolrServer("", baseUrl).request(request);
SimpleOrderedMap success = (SimpleOrderedMap) resp.get("success");
SimpleOrderedMap failure = (SimpleOrderedMap) resp.get("failure");
String val1 = success.getVal(0).toString();
String val2 = failure.getVal(0).toString();
assertTrue(val1.contains("SolrException") || val2.contains("SolrException"));
}
private void testNodesUsedByCreate() throws Exception {
// we can use this client because we just want base url
final String baseUrl = getBaseUrl((HttpSolrServer) clients.get(0));
@ -324,7 +432,13 @@ public class CollectionsAPIDistributedZkTest extends AbstractFullDistribZkTestBa
request = new QueryRequest(params);
request.setPath("/admin/collections");
createNewSolrServer("", baseUrl).request(request);
boolean exp = false;
try {
createNewSolrServer("", baseUrl).request(request);
} catch (SolrException e) {
exp = true;
}
assertTrue("Expected exception", exp);
// create another collection should still work
params = new ModifiableSolrParams();
@ -363,14 +477,17 @@ public class CollectionsAPIDistributedZkTest extends AbstractFullDistribZkTestBa
collectionInfos = new HashMap<String,List<Integer>>();
CloudSolrServer client = createCloudClient("awholynewcollection_" + cnt);
try {
exp = false;
try {
createCollection(collectionInfos, "awholynewcollection_" + cnt, numShards, replicationFactor, maxShardsPerNode, client, null);
} catch (SolrException e) {
exp = true;
}
assertTrue("expected exception", exp);
} finally {
client.shutdown();
}
// TODO: REMOVE THE SLEEP IN THE METHOD CALL WHEN WE HAVE COLLECTION API
// RESPONSES
checkCollectionIsNotCreated(collectionInfos.keySet().iterator().next());
// Test createNodeSet
numLiveNodes = getCommonCloudSolrServer().getZkStateReader().getClusterState().getLiveNodes().size();
@ -509,13 +626,6 @@ public class CollectionsAPIDistributedZkTest extends AbstractFullDistribZkTestBa
fail("Could not find the new collection - " + exp.code() + " : " + collectionClient.getBaseURL());
}
private void checkCollectionIsNotCreated(String collectionName)
throws Exception {
// TODO: REMOVE THIS SLEEP WHEN WE HAVE COLLECTION API RESPONSES
Thread.sleep(10000);
assertFalse(collectionName + " not supposed to exist", getCommonCloudSolrServer().getZkStateReader().getClusterState().getCollections().contains(collectionName));
}
private void checkForMissingCollection(String collectionName)
throws Exception {
// check for a collection - we poll the state

View File

@ -38,6 +38,7 @@ import java.util.Set;
import org.apache.solr.SolrTestCaseJ4;
import org.apache.solr.client.solrj.SolrResponse;
import org.apache.solr.client.solrj.response.QueryResponse;
import org.apache.solr.cloud.DistributedQueue.QueueEvent;
import org.apache.solr.common.cloud.ClusterState;
import org.apache.solr.common.cloud.SolrZkClient;
@ -46,7 +47,6 @@ import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.common.params.CoreAdminParams;
import org.apache.solr.common.params.CoreAdminParams.CoreAdminAction;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.common.util.StrUtils;
import org.apache.solr.handler.component.ShardHandler;
import org.apache.solr.handler.component.ShardRequest;
@ -248,6 +248,7 @@ public class OverseerCollectionProcessorTest extends SolrTestCaseJ4 {
expectLastCall();
submitCaptures.add(submitCapture);
ShardResponse shardResponseWithoutException = new ShardResponse();
shardResponseWithoutException.setSolrResponse(new QueryResponse());
expect(shardHandlerMock.takeCompletedOrError()).andReturn(
shardResponseWithoutException);
}

View File

@ -23,6 +23,9 @@ import org.apache.solr.SolrIgnoredThreadsFilter;
import org.apache.solr.client.solrj.SolrServer;
import org.apache.solr.client.solrj.embedded.AbstractEmbeddedSolrServerTestCase;
import org.apache.solr.client.solrj.embedded.EmbeddedSolrServer;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.params.ModifiableSolrParams;
import org.apache.solr.common.util.NamedList;
import org.apache.solr.core.SolrCore;
import org.apache.commons.io.FileUtils;
import org.junit.After;
@ -94,6 +97,26 @@ public class TestCoreAdmin extends AbstractEmbeddedSolrServerTestCase {
}
@Test
public void testErrorCases() throws Exception {
ModifiableSolrParams params = new ModifiableSolrParams();
params.set("action", "BADACTION");
String collectionName = "badactioncollection";
params.set("name", collectionName);
QueryRequest request = new QueryRequest(params);
request.setPath("/admin/cores");
boolean gotExp = false;
NamedList<Object> resp = null;
try {
resp = getSolrAdmin().request(request);
} catch (SolrException e) {
gotExp = true;
}
assertTrue(gotExp);
}
@BeforeClass
public static void before() {
// wtf?