You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by ds...@apache.org on 2016/03/19 02:29:38 UTC

[3/3] lucene-solr:solr-5750: SOLR-5750: test passes but more nocommits. Incl fixes in Overseer & ClusterStateMutator

SOLR-5750: test passes but more nocommits. Incl fixes in Overseer & ClusterStateMutator


Project: http://git-wip-us.apache.org/repos/asf/lucene-solr/repo
Commit: http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/fd9c4d59
Tree: http://git-wip-us.apache.org/repos/asf/lucene-solr/tree/fd9c4d59
Diff: http://git-wip-us.apache.org/repos/asf/lucene-solr/diff/fd9c4d59

Branch: refs/heads/solr-5750
Commit: fd9c4d59e8dbe0e9fbd99a40ed2ff746c1ae7a0c
Parents: 31a28f3
Author: David Smiley <ds...@apache.org>
Authored: Fri Mar 18 21:29:12 2016 -0400
Committer: David Smiley <ds...@apache.org>
Committed: Fri Mar 18 21:29:12 2016 -0400

----------------------------------------------------------------------
 .../java/org/apache/solr/cloud/Overseer.java    |  17 +-
 .../cloud/OverseerCollectionMessageHandler.java | 192 +++++++++----------
 .../cloud/overseer/ClusterStateMutator.java     |   2 +-
 .../solr/cloud/TestCloudBackupRestore.java      |  91 ++++-----
 .../solrj/request/CollectionAdminRequest.java   |  82 ++++----
 .../apache/solr/common/cloud/ZkStateReader.java |   4 +-
 .../solr/cloud/AbstractDistribZkTestBase.java   |   2 +-
 .../cloud/AbstractFullDistribZkTestBase.java    |   2 +-
 8 files changed, 199 insertions(+), 193 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/fd9c4d59/solr/core/src/java/org/apache/solr/cloud/Overseer.java
----------------------------------------------------------------------
diff --git a/solr/core/src/java/org/apache/solr/cloud/Overseer.java b/solr/core/src/java/org/apache/solr/cloud/Overseer.java
index ec701c3..7244514 100644
--- a/solr/core/src/java/org/apache/solr/cloud/Overseer.java
+++ b/solr/core/src/java/org/apache/solr/cloud/Overseer.java
@@ -140,6 +140,7 @@ public class Overseer implements Closeable {
       try {
         ZkStateWriter zkStateWriter = null;
         ClusterState clusterState = null;
+        //nocommit: can't this simply be the same as clusterState being null?
         boolean refreshClusterState = true; // let's refresh in the first iteration
         while (!this.isClosed) {
           isLeader = amILeader();
@@ -155,6 +156,7 @@ public class Overseer implements Closeable {
             try {
               reader.updateClusterState();
               clusterState = reader.getClusterState();
+              assert clusterState != null : "should clusterState be null?";//nocommit
               zkStateWriter = new ZkStateWriter(reader, stats);
               refreshClusterState = false;
 
@@ -169,12 +171,14 @@ public class Overseer implements Closeable {
                 // force flush to ZK after each message because there is no fallback if workQueue items
                 // are removed from workQueue but fail to be written to ZK
                 clusterState = processQueueItem(message, clusterState, zkStateWriter, false, null);
+                assert clusterState != null : "should clusterState be null?";//nocommit
                 workQueue.poll(); // poll-ing removes the element we got by peek-ing
                 data = workQueue.peek();
               }
               // force flush at the end of the loop
               if (hadWorkItems) {
                 clusterState = zkStateWriter.writePendingUpdates();
+                assert clusterState != null : "should clusterState be null?";//nocommit
               }
             } catch (KeeperException e) {
               if (e.code() == KeeperException.Code.SESSIONEXPIRED) {
@@ -188,6 +192,8 @@ public class Overseer implements Closeable {
             } catch (Exception e) {
               log.error("Exception in Overseer work queue loop", e);
             }
+          } else {
+            assert clusterState != null : "should clusterState be null?";//nocommit
           }
 
           byte[] head = null;
@@ -225,6 +231,7 @@ public class Overseer implements Closeable {
                   while (workQueue.poll() != null);
                 }
               });
+              assert clusterState != null : "should clusterState be null?";//nocommit
 
               // it is safer to keep this poll here because an invalid message might never be queued
               // and therefore we can't rely on the ZkWriteCallback to remove the item
@@ -236,7 +243,13 @@ public class Overseer implements Closeable {
             }
             // we should force write all pending updates because the next iteration might sleep until there
             // are more items in the main queue
-            clusterState = zkStateWriter.writePendingUpdates();
+
+            ClusterState clusterState2 = zkStateWriter.writePendingUpdates();//note: may return null?
+            if (clusterState2 == null) {
+              log.error("zkStateWriter.writePendingUpdates returned null clusterState");
+            } else {
+              clusterState = clusterState2;
+            }
             // clean work queue
             while (workQueue.poll() != null);
 
@@ -369,7 +382,7 @@ public class Overseer implements Closeable {
             return Collections.singletonList(new CollectionMutator(reader).modifyCollection(clusterState, message));
           case MIGRATESTATEFORMAT:
             return Collections.singletonList(new ClusterStateMutator(reader).migrateStateFormat(clusterState, message));
-          case RESTORE:
+          case RESTORE: // nocommit needs explaination
             break;
           case BACKUP:
             break;

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/fd9c4d59/solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java
----------------------------------------------------------------------
diff --git a/solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java b/solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java
index 9c79470..8a35b6d 100644
--- a/solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java
+++ b/solr/core/src/java/org/apache/solr/cloud/OverseerCollectionMessageHandler.java
@@ -17,8 +17,10 @@
 package org.apache.solr.cloud;
 
 import java.io.FileInputStream;
+import java.io.FileNotFoundException;
 import java.io.FileOutputStream;
 import java.io.IOException;
+import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.OutputStreamWriter;
 import java.lang.invoke.MethodHandles;
@@ -50,6 +52,7 @@ import org.apache.solr.client.solrj.SolrServerException;
 import org.apache.solr.client.solrj.impl.HttpSolrClient;
 import org.apache.solr.client.solrj.impl.HttpSolrClient.RemoteSolrException;
 import org.apache.solr.client.solrj.request.AbstractUpdateRequest;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
 import org.apache.solr.client.solrj.request.CoreAdminRequest;
 import org.apache.solr.client.solrj.request.UpdateRequest;
 import org.apache.solr.client.solrj.response.UpdateResponse;
@@ -335,46 +338,27 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler
     String location = message.getStr("location");
     ShardHandler shardHandler = shardHandlerFactory.getShardHandler();
     final String asyncId = message.getStr(ASYNC);
-    Map<String, String> requestMap = null;
-    if (asyncId != null) {
-      requestMap = new HashMap<>();
-    }
-    Path backupPath = Paths.get(location).resolve(name).toAbsolutePath();
+    Map<String, String> requestMap = new HashMap<>();
 
-    if (!Files.exists(backupPath)) {
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
-          "Backup directory does not exist: " + backupPath.toString());
-    }
+    Path backupPath = Paths.get(location).resolve(name).toAbsolutePath();
 
-    Path zkBackup =  backupPath.resolve("zk_backup");
-    if (!Files.exists(zkBackup)) {
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
-          "Backup zk directory does not exist: " + backupPath.toString());
-    }
+    Path backupZkPath =  backupPath.resolve("zk_backup");
 
-    Path propertiesPath = zkBackup.resolve("backup.properties");
-    if (!Files.exists(propertiesPath)) {
-      throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
-          "backup.properties file does not exist: " + backupPath.toString());
-    }
+    Path backupZkPropsPath = backupZkPath.resolve("backup.properties");
 
-    FileInputStream in = null;
-    Properties properties = null;
-    try {
-      in = new FileInputStream(propertiesPath.toAbsolutePath().toString());
-      properties = new Properties();
+    Properties properties = new Properties();
+    try (InputStream in = Files.newInputStream(backupZkPropsPath)) {
       properties.load(new InputStreamReader(in, StandardCharsets.UTF_8));
     } catch (IOException e) {
       String errorMsg = String.format(Locale.ROOT, "Could not load properties from %s: %s:",
-          propertiesPath.toAbsolutePath().toString(), e.toString());
+          backupZkPropsPath.toAbsolutePath().toString(), e.toString());
       throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, errorMsg);
-    } finally {
-      IOUtils.closeQuietly(in);
     }
 
     String backupCollection = (String) properties.get("collectionName");
-    Path collectionStatePath = zkBackup.resolve("collection_state_backup.json");
+    Path collectionStatePath = backupZkPath.resolve("collection_state_backup.json");
     byte[] data = Files.readAllBytes(collectionStatePath);
+    @SuppressWarnings("unchecked")
     Map<String, Object> collectionProps = (Map<String, Object>) ((Map<String, Object>) Utils.fromJSON(data)).get(backupCollection);
 
     //Download the configs
@@ -383,42 +367,46 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler
     //Use a name such as restore.<restore_name>.<original_config_name>
     // in ZK for the configs
     String restoreConfigName = "restore." + configName;
-    zkStateReader.getConfigManager().uploadConfigDir(zkBackup.resolve("configs").resolve(configName), restoreConfigName);
+    zkStateReader.getConfigManager().uploadConfigDir(backupZkPath.resolve("configs").resolve(configName), restoreConfigName);
 
     log.debug("Starting restore into collection={} with backup_name={} at location={}", restoreCollectionName, name,
-        backupPath.toString());
+        backupPath);
 
     //Create core-less collection
-    Map<String, Object> propMap = new HashMap<>();
-    propMap.put(NAME, restoreCollectionName);
-    propMap.put(CREATE_NODE_SET, CREATE_NODE_SET_EMPTY); //no cores
-    propMap.put("collection.configName", restoreConfigName);
-    // add async param
-    if (asyncId != null) {
-      propMap.put(ASYNC, asyncId);
-    }
-    String router = (String) ((Map)collectionProps.get("router")).get("name");
-    propMap.put("router.name", router);
-    Map slices = (Map) collectionProps.get(SHARDS_PROP);
-    if (slices != null) { //Implicit routers may not have a shards defined
-      propMap.put(NUM_SLICES, slices.size());
-    }
-    if (ImplicitDocRouter.NAME.equals(router)) {
-      Iterator keys = ((Map) collectionProps.get(SHARDS_PROP)).keySet().iterator();
-      StringBuilder shardsBuilder = new StringBuilder();
-      while (keys.hasNext()) {
-        String shard = (String) keys.next();
-        shardsBuilder.append(shard);
-        shardsBuilder.append(",");
+    {
+      Map<String, Object> propMap = new HashMap<>();
+      propMap.put(NAME, restoreCollectionName);
+      propMap.put(CREATE_NODE_SET, CREATE_NODE_SET_EMPTY); //no cores
+      propMap.put("collection.configName", restoreConfigName);
+      // add async param
+      if (asyncId != null) {
+        propMap.put(ASYNC, asyncId);
       }
-      String shards = shardsBuilder.deleteCharAt(shardsBuilder.length()-1).toString();
-      propMap.put(SHARDS_PROP, shards);
-    }
-    propMap.put(MAX_SHARDS_PER_NODE, Integer.parseInt((String) collectionProps.get(MAX_SHARDS_PER_NODE)));
-    propMap.put(ZkStateReader.AUTO_ADD_REPLICAS, Boolean.parseBoolean((String) collectionProps.get(ZkStateReader.AUTO_ADD_REPLICAS)));
-    propMap.put(Overseer.QUEUE_OPERATION, CREATE.toString());
+      String router = (String) ((Map)collectionProps.get("router")).get("name");
+      propMap.put("router.name", router);
+      Map slices = (Map) collectionProps.get(SHARDS_PROP);
+      if (slices != null) { //Implicit routers may not have a shards defined
+        propMap.put(NUM_SLICES, slices.size());
+      }
+      if (ImplicitDocRouter.NAME.equals(router)) {
+        Iterator keys = ((Map) collectionProps.get(SHARDS_PROP)).keySet().iterator();
+        StringBuilder shardsBuilder = new StringBuilder();
+        while (keys.hasNext()) {
+          String shard = (String) keys.next();
+          shardsBuilder.append(shard);
+          shardsBuilder.append(",");
+        }
+        String shards = shardsBuilder.deleteCharAt(shardsBuilder.length()-1).toString();//delete trailing comma
+        propMap.put(SHARDS_PROP, shards);
+      }
+      // TODO nocommit loop all from a list and blacklist those we know could never work.
+      //     The user could always edit the properties file if they need to.
+      propMap.put(MAX_SHARDS_PER_NODE, Integer.parseInt((String) collectionProps.get(MAX_SHARDS_PER_NODE)));
+      propMap.put(ZkStateReader.AUTO_ADD_REPLICAS, Boolean.parseBoolean((String) collectionProps.get(ZkStateReader.AUTO_ADD_REPLICAS)));
+      propMap.put(Overseer.QUEUE_OPERATION, CREATE.toString());
 
-    createCollection(zkStateReader.getClusterState(), new ZkNodeProps(propMap), new NamedList());
+      createCollection(zkStateReader.getClusterState(), new ZkNodeProps(propMap), new NamedList());
+    }
 
     //No need to wait. CreateCollection takes care of it by calling waitToSeeReplicasInState()
     DocCollection restoreCollection = zkStateReader.getClusterState().getCollection(restoreCollectionName);
@@ -426,21 +414,24 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler
       throw new SolrException(ErrorCode.SERVER_ERROR, "Could not create restore collection");
     }
 
-    //Mark all shards in CONSTRUCTION STATE while we restore the data
     DistributedQueue inQueue = Overseer.getStateUpdateQueue(zkStateReader.getZkClient());
-    propMap = new HashMap<>();
-    propMap.put(Overseer.QUEUE_OPERATION, OverseerAction.UPDATESHARDSTATE.toLower());
-    for (Slice shard : restoreCollection.getSlices()) {
-      propMap.put(shard.getName(), Slice.State.CONSTRUCTION.toString());
+
+    //Mark all shards in CONSTRUCTION STATE while we restore the data
+    {
+      Map<String, Object> propMap = new HashMap<>();
+      propMap.put(Overseer.QUEUE_OPERATION, OverseerAction.UPDATESHARDSTATE.toLower());
+      for (Slice shard : restoreCollection.getSlices()) {
+        propMap.put(shard.getName(), Slice.State.CONSTRUCTION.toString());
+      }
+      propMap.put(ZkStateReader.COLLECTION_PROP, restoreCollectionName);
+      inQueue.offer(Utils.toJSON(new ZkNodeProps(propMap)));
     }
-    propMap.put(ZkStateReader.COLLECTION_PROP, restoreCollectionName);
-    inQueue.offer(Utils.toJSON(new ZkNodeProps(propMap)));
 
     ClusterState clusterState = zkStateReader.getClusterState();
     //Create one replica per shard and copy backed up data to it
     for (Slice slice: restoreCollection.getSlices()) {
       log.debug("Adding replica for shard={} collection={} ", slice.getName(), restoreCollection);
-      propMap = new HashMap<>();
+      HashMap<String, Object> propMap = new HashMap<>();
       propMap.put(Overseer.QUEUE_OPERATION, ADDREPLICA.toLower());
       propMap.put(COLLECTION_PROP, restoreCollectionName);
       propMap.put(SHARD_ID_PROP, slice.getName());
@@ -465,50 +456,55 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler
     processResponses(new NamedList(), shardHandler, true, "Could not restore core", asyncId, requestMap);
 
     //Mark all shards in ACTIVE STATE
-    propMap = new HashMap<>();
-    propMap.put(Overseer.QUEUE_OPERATION, OverseerAction.UPDATESHARDSTATE.toLower());
-    propMap.put(ZkStateReader.COLLECTION_PROP, restoreCollectionName);
-    for (Slice shard : restoreCollection.getSlices()) {
-      propMap.put(shard.getName(), Slice.State.ACTIVE.toString());
+
+    {
+      HashMap<String, Object> propMap = new HashMap<>();
+      propMap.put(Overseer.QUEUE_OPERATION, OverseerAction.UPDATESHARDSTATE.toLower());
+      propMap.put(ZkStateReader.COLLECTION_PROP, restoreCollectionName);
+      for (Slice shard : restoreCollection.getSlices()) {
+        propMap.put(shard.getName(), Slice.State.ACTIVE.toString());
+      }
+      inQueue.offer(Utils.toJSON(new ZkNodeProps(propMap)));
     }
-    inQueue.offer(Utils.toJSON(new ZkNodeProps(propMap)));
 
     //refresh the location copy of collection state
     restoreCollection = zkStateReader.getClusterState().getCollection(restoreCollectionName);
 
     //Update the replicationFactor to be 1 as that's what it is currently. Otherwise addreplica assigns wrong core names
-    propMap = new HashMap<>();
-    propMap.put(Overseer.QUEUE_OPERATION, CollectionParams.CollectionAction.MODIFYCOLLECTION.toLower());
-    propMap.put(COLLECTION_PROP, restoreCollectionName);
-    propMap.put(REPLICATION_FACTOR, 1);
-    inQueue.offer(Utils.toJSON(message));
+    {
+      HashMap<String, Object> propMap = new HashMap<>();
+      propMap.put(Overseer.QUEUE_OPERATION, CollectionParams.CollectionAction.MODIFYCOLLECTION.toLower());
+      propMap.put(COLLECTION_PROP, restoreCollectionName);
+      propMap.put(REPLICATION_FACTOR, 1);
+      inQueue.offer(Utils.toJSON(new ZkNodeProps(propMap)));
+    }
 
     //Add the remaining replicas for each shard
     int numReplicas = Integer.parseInt((String) collectionProps.get(REPLICATION_FACTOR));
-    for (Slice slice: restoreCollection.getSlices()) {
-      for(int i=1; i<numReplicas; i++) {
-        log.debug("Adding replica for shard={} collection={} ", slice.getName(), restoreCollection);
-        propMap = new HashMap<>();
-        propMap.put(COLLECTION_PROP, restoreCollectionName);
-        propMap.put(SHARD_ID_PROP, slice.getName());
-        // add async param
-        if (asyncId != null) {
-          propMap.put(ASYNC, asyncId);
+    if (numReplicas > 1) {
+      for (Slice slice: restoreCollection.getSlices()) {
+        for(int i=1; i<numReplicas; i++) {
+          log.debug("Adding replica for shard={} collection={} ", slice.getName(), restoreCollection);
+          HashMap<String, Object> propMap = new HashMap<>();
+          propMap.put(COLLECTION_PROP, restoreCollectionName);
+          propMap.put(SHARD_ID_PROP, slice.getName());
+          // add async param
+          if (asyncId != null) {
+            propMap.put(ASYNC, asyncId);
+          }
+          addReplica(zkStateReader.getClusterState(), new ZkNodeProps(propMap), results);
         }
-        addReplica(zkStateReader.getClusterState(), new ZkNodeProps(propMap), results);
       }
-    }
 
-    if (numReplicas > 1) {
       //Update the replicationFactor property in cluster state for this collection
       log.info("Modifying replication factor to the expected value of={}", numReplicas);
-      propMap = new HashMap<>();
+      HashMap<String, Object>propMap = new HashMap<>();
       propMap.put(Overseer.QUEUE_OPERATION, CollectionParams.CollectionAction.MODIFYCOLLECTION.toLower());
       propMap.put(COLLECTION_PROP, restoreCollectionName);
       propMap.put(REPLICATION_FACTOR, numReplicas);
-      inQueue.offer(Utils.toJSON(message));
+      inQueue.offer(Utils.toJSON(new ZkNodeProps(propMap)));
+      //nocommit do we need to wait for the result to be done before returning?
     }
-
   }
 
   private void processBackupAction(ZkNodeProps message, NamedList results) throws IOException, KeeperException, InterruptedException {
@@ -527,11 +523,11 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler
     //Validating if the directory already exists.
     if (Files.exists(backupPath)) {
       throw new SolrException(SolrException.ErrorCode.BAD_REQUEST,
-          "Backup directory already exists: " + backupPath.toString());
+          "Backup directory already exists: " + backupPath);
     }
 
     log.debug("Starting backup of collection={} with backup_name={} at location={}", collectionName, name,
-        backupPath.toString());
+        backupPath);
 
     for (Slice slice : zkStateReader.getClusterState().getActiveSlices(collectionName)) {
       Replica replica = slice.getLeader();
@@ -575,16 +571,14 @@ public class OverseerCollectionMessageHandler implements OverseerMessageHandler
     //TODO: Add MD5 of the configset. If during restore the same name configset exists then we can compare checksums to see if they are the same.
     //if they are not the same then we can throw and error or have a 'overwriteConfig' flag
     //TODO save numDocs for the shardLeader. We can use it to sanity check the restore.
-    OutputStreamWriter os = new OutputStreamWriter(new FileOutputStream(propertiesPath.toAbsolutePath().toString()), StandardCharsets.UTF_8);
 
-    try {
+    try (OutputStreamWriter os = new OutputStreamWriter(
+        new FileOutputStream(propertiesPath.toAbsolutePath().toString()), StandardCharsets.UTF_8)) {
       properties.store(os, "Snapshot properties file");
     } catch (IOException e) {
       String errorMsg = String.format(Locale.ROOT, "Could not write properties to %s: %s:",
           propertiesPath.toAbsolutePath().toString(), e.toString());
-      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, errorMsg);
-    } finally {
-      IOUtils.closeQuietly(os);
+      throw new SolrException(ErrorCode.SERVER_ERROR, errorMsg);
     }
 
     log.debug("Completed backing up ZK data for backup={}", name);

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/fd9c4d59/solr/core/src/java/org/apache/solr/cloud/overseer/ClusterStateMutator.java
----------------------------------------------------------------------
diff --git a/solr/core/src/java/org/apache/solr/cloud/overseer/ClusterStateMutator.java b/solr/core/src/java/org/apache/solr/cloud/overseer/ClusterStateMutator.java
index 7ffa8c1..6864742 100644
--- a/solr/core/src/java/org/apache/solr/cloud/overseer/ClusterStateMutator.java
+++ b/solr/core/src/java/org/apache/solr/cloud/overseer/ClusterStateMutator.java
@@ -101,7 +101,7 @@ public class ClusterStateMutator {
       collectionProps.put("autoCreated", "true");
     }
 
-    String znode = message.getInt(DocCollection.STATE_FORMAT, 1) == 1 ? null
+    String znode = message.getInt(DocCollection.STATE_FORMAT, 2) == 1 ? null
         : ZkStateReader.getCollectionPath(cName);
 
     DocCollection newCollection = new DocCollection(cName,

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/fd9c4d59/solr/core/src/test/org/apache/solr/cloud/TestCloudBackupRestore.java
----------------------------------------------------------------------
diff --git a/solr/core/src/test/org/apache/solr/cloud/TestCloudBackupRestore.java b/solr/core/src/test/org/apache/solr/cloud/TestCloudBackupRestore.java
index a2a01a7..7dbefda 100644
--- a/solr/core/src/test/org/apache/solr/cloud/TestCloudBackupRestore.java
+++ b/solr/core/src/test/org/apache/solr/cloud/TestCloudBackupRestore.java
@@ -17,49 +17,32 @@
 
 package org.apache.solr.cloud;
 
-import java.io.File;
-
 import org.apache.lucene.util.TestUtil;
-import org.apache.solr.SolrTestCaseJ4;
 import org.apache.solr.client.solrj.SolrQuery;
 import org.apache.solr.client.solrj.impl.CloudSolrClient;
 import org.apache.solr.client.solrj.request.CollectionAdminRequest;
-import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrInputDocument;
-import org.apache.solr.common.cloud.ClusterState;
 import org.apache.solr.common.cloud.DocCollection;
 import org.apache.solr.common.cloud.ImplicitDocRouter;
 import org.apache.solr.common.util.NamedList;
-import org.junit.After;
-import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import static org.apache.solr.common.params.ShardParams._ROUTE_;
 
-public class TestCloudBackupRestore extends SolrTestCaseJ4 {
-
-  static Logger log = LoggerFactory.getLogger(TestCloudBackupRestore.class);
+public class TestCloudBackupRestore extends SolrCloudTestCase {
 
-  private MiniSolrCloudCluster solrCluster;
+  private static Logger log = LoggerFactory.getLogger(TestCloudBackupRestore.class);
 
-  @Override
-  @Before
-  public void setUp() throws Exception {
-    super.setUp();
-    solrCluster = new MiniSolrCloudCluster(2, createTempDir(), buildJettyConfig("/solr"));
-    final File configDir = getFile("solr").toPath().resolve("collection1/conf").toFile();
-    solrCluster.uploadConfigDir(configDir, "conf1");
-    System.setProperty("solr.test.sys.prop1", "propone");
-    System.setProperty("solr.test.sys.prop2", "proptwo");
-  }
+  private static final int NUM_SHARDS = 2;
 
-  @Override
-  @After
-  public void tearDown() throws Exception {
-    solrCluster.shutdown();
-    super.tearDown();
+  @BeforeClass
+  public static void createCluster() throws Exception {
+    configureCluster(2)
+        .addConfig("conf1", TEST_PATH().resolve("configsets").resolve("cloud-minimal").resolve("conf"))
+        .configure();
   }
 
   @Test
@@ -67,28 +50,26 @@ public class TestCloudBackupRestore extends SolrTestCaseJ4 {
     String collectionName = "backuprestore";
     String restoreCollectionName = collectionName + "_restored";
     boolean isImplicit = random().nextBoolean();
-    CollectionAdminRequest.Create create = new CollectionAdminRequest.Create()
-        .setCollectionName(collectionName)
-        .setConfigName("conf1")
-        .setReplicationFactor(TestUtil.nextInt(random(), 1, 2))
-        .setMaxShardsPerNode(2);
+    int numReplicas = TestUtil.nextInt(random(), 1, 2);
+    CollectionAdminRequest.Create create =
+        CollectionAdminRequest.createCollection(collectionName, "conf1", NUM_SHARDS, numReplicas);
+    create.setMaxShardsPerNode(NUM_SHARDS);
     if (isImplicit) { //implicit router
       create.setRouterName(ImplicitDocRouter.NAME);
-      create.setShards("shard1,shard2");
+      create.setNumShards(null);//erase it
+      create.setShards("shard1,shard2"); // however still same number as NUM_SHARDS; we assume this later
       create.setRouterField("shard_s");
-    } else {
-      create.setNumShards(2);
     }
-
-    create.process(solrCluster.getSolrClient());
-    AbstractDistribZkTestBase.waitForRecoveriesToFinish("backuprestore", solrCluster.getSolrClient().getZkStateReader(), false, true, 30);
+//TODO nocommit test shard split & custom doc route?
+    create.process(cluster.getSolrClient());
+    waitForCollection(collectionName);
     indexDocs(collectionName);
     testBackupAndRestore(collectionName, restoreCollectionName, isImplicit);
   }
 
   private void indexDocs(String collectionName) throws Exception {
     int numDocs = TestUtil.nextInt(random(), 10, 100);
-    CloudSolrClient client = solrCluster.getSolrClient();
+    CloudSolrClient client = cluster.getSolrClient();
     client.setDefaultCollection(collectionName);
     for (int i=0; i<numDocs; i++) {
       //We index the shard_s fields for whichever router gets chosen but only use it when implicit router was selected
@@ -109,7 +90,7 @@ public class TestCloudBackupRestore extends SolrTestCaseJ4 {
 
   private void testBackupAndRestore(String collectionName, String restoreCollectionName, boolean isImplicit) throws Exception {
     String backupName = "mytestbackup";
-    CloudSolrClient client = solrCluster.getSolrClient();
+    CloudSolrClient client = cluster.getSolrClient();
     long totalDocs = client.query(collectionName, new SolrQuery("*:*")).getResults().getNumFound();
     long shard1Docs = 0, shard2Docs = 0;
     if (isImplicit) {
@@ -121,29 +102,19 @@ public class TestCloudBackupRestore extends SolrTestCaseJ4 {
     String location = createTempDir().toFile().getAbsolutePath();
 
     log.info("Triggering Backup command");
-    //Run backup command
-    CollectionAdminRequest.Backup backup = new CollectionAdminRequest.Backup(backupName, collectionName)
+
+    CollectionAdminRequest.Backup backup = CollectionAdminRequest.backupCollection(collectionName, backupName)
         .setLocation(location);
-    NamedList<Object> rsp = solrCluster.getSolrClient().request(backup);
+    NamedList<Object> rsp = cluster.getSolrClient().request(backup);
     assertEquals(0, ((NamedList)rsp.get("responseHeader")).get("status"));
 
     log.info("Triggering Restore command");
 
-    //Restore
-    CollectionAdminRequest.Restore restore = new CollectionAdminRequest.Restore(backupName, restoreCollectionName)
+    CollectionAdminRequest.Restore restore = CollectionAdminRequest.restoreCollection(restoreCollectionName, backupName)
         .setLocation(location);
-    rsp = solrCluster.getSolrClient().request(restore);
+    rsp = cluster.getSolrClient().request(restore);
     assertEquals(0, ((NamedList)rsp.get("responseHeader")).get("status"));
-
-    client.getZkStateReader().updateClusterState();
-    DocCollection restoreCollection = null;
-    while (restoreCollection == null)  {
-      try {
-        restoreCollection = client.getZkStateReader().getClusterState().getCollection(restoreCollectionName);
-      } catch (SolrException e) {
-        Thread.sleep(100); //wait for cluster state to update
-      }
-    }
+    waitForCollection(restoreCollectionName);
 
     //Check the number of results are the same
     long restoredNumDocs = client.query(restoreCollectionName, new SolrQuery("*:*")).getResults().getNumFound();
@@ -158,9 +129,17 @@ public class TestCloudBackupRestore extends SolrTestCaseJ4 {
     }
 
     DocCollection backupCollection = client.getZkStateReader().getClusterState().getCollection(collectionName);
+    DocCollection restoreCollection = client.getZkStateReader().getClusterState().getCollection(restoreCollectionName);
+
     assertEquals(backupCollection.getReplicationFactor(), restoreCollection.getReplicationFactor());
 
-    assertEquals( "restore.conf1", solrCluster.getSolrClient().getZkStateReader().readConfigName(restoreCollectionName));
+    assertEquals("restore.conf1", cluster.getSolrClient().getZkStateReader().readConfigName(restoreCollectionName));
+  }
+
+  public void waitForCollection(String collection) throws Exception {
+    AbstractFullDistribZkTestBase.waitForCollection(cluster.getSolrClient().getZkStateReader(), collection, NUM_SHARDS);
+    AbstractDistribZkTestBase.waitForRecoveriesToFinish(collection, cluster.getSolrClient().getZkStateReader(), log.isDebugEnabled(), true, 30);
+    AbstractDistribZkTestBase.assertAllActive(collection, cluster.getSolrClient().getZkStateReader());
   }
 
 }

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/fd9c4d59/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
----------------------------------------------------------------------
diff --git a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
index f01bede..c18900b 100644
--- a/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
+++ b/solr/solrj/src/java/org/apache/solr/client/solrj/request/CollectionAdminRequest.java
@@ -527,11 +527,33 @@ public abstract class CollectionAdminRequest<T extends CollectionAdminResponse>
     }
   }
 
+  public static Backup backupCollection(String collection, String backupName) {
+    return new Backup(collection, backupName);
+  }
+
   // BACKUP request
-  public static class Backup extends AsyncCollectionAdminRequest<Backup> {
+  public static class Backup extends AsyncCollectionSpecificAdminRequest {
+    protected final String name;
     protected String location;
-    protected String name;
-    protected String collection;
+
+    public Backup(String collection, String name) {
+      super(CollectionAction.BACKUP, collection);
+      this.name = name;
+    }
+
+    @Override
+    @Deprecated
+    public Backup setAsyncId(String id) {
+      this.asyncId = id;
+      return this;
+    }
+
+    @Override
+    @Deprecated
+    public Backup setCollectionName(String collection) {
+      this.collection = collection;
+      return this;
+    }
 
     public String getLocation() {
       return location;
@@ -542,34 +564,42 @@ public abstract class CollectionAdminRequest<T extends CollectionAdminResponse>
       return this;
     }
 
-    public Backup(String name, String collection) {
-      this.name = name;
-      this.collection = collection;
-      action = CollectionAction.BACKUP;
-    }
-
     @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = (ModifiableSolrParams) super.getParams();
       params.set(CoreAdminParams.COLLECTION, collection);
       params.set(CoreAdminParams.NAME, name);
-      if (location != null) {
-        params.set("location", location);
-      }
+      params.set("location", location); //note: optional
       return params;
     }
 
-    @Override
-    protected Backup getThis() {
-      return this;
-    }
+  }
+
+  public static Restore restoreCollection(String collection, String backupName) {
+    return new Restore(collection, backupName);
   }
 
   // RESTORE request
-  public static class Restore extends AsyncCollectionAdminRequest<Restore> {
+  public static class Restore extends AsyncCollectionSpecificAdminRequest {
+    protected final String name;
     protected String location;
-    protected String name;
-    protected String collection;
+
+    public Restore(String collection, String name) {
+      super(CollectionAction.RESTORE, collection);
+      this.name = name;
+    }
+
+    @Override
+    public Restore setAsyncId(String id) {
+      this.asyncId = id;
+      return this;
+    }
+
+    @Override
+    public Restore setCollectionName(String collection) {
+      this.collection = collection;
+      return this;
+    }
 
     public String getLocation() {
       return location;
@@ -580,27 +610,15 @@ public abstract class CollectionAdminRequest<T extends CollectionAdminResponse>
       return this;
     }
 
-    public Restore(String name, String collection) {
-      this.name = name;
-      this.collection = collection;
-      action = CollectionAction.RESTORE;
-    }
-
     @Override
     public SolrParams getParams() {
       ModifiableSolrParams params = (ModifiableSolrParams) super.getParams();
       params.set(CoreAdminParams.COLLECTION, collection);
       params.set(CoreAdminParams.NAME, name);
-      if (location != null) {
-        params.set("location", location);
-      }
+      params.set("location", location); //note: optional
       return params;
     }
 
-    @Override
-    protected Restore getThis() {
-      return this;
-    }
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/fd9c4d59/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
----------------------------------------------------------------------
diff --git a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
index 37c275b..a001af0 100644
--- a/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
+++ b/solr/solrj/src/java/org/apache/solr/common/cloud/ZkStateReader.java
@@ -97,6 +97,8 @@ public class ZkStateReader implements Closeable {
 
   public static final String URL_SCHEME = "urlScheme";
 
+  public static final String BACKUP_LOCATION = "location";
+
   /** A view of the current state of all collections; combines all the different state sources into a single view. */
   protected volatile ClusterState clusterState;
 
@@ -134,7 +136,7 @@ public class ZkStateReader implements Closeable {
       LEGACY_CLOUD,
       URL_SCHEME,
       AUTO_ADD_REPLICAS,
-      "location")));
+      BACKUP_LOCATION)));
 
   /**
    * Returns config set name for collection.

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/fd9c4d59/solr/test-framework/src/java/org/apache/solr/cloud/AbstractDistribZkTestBase.java
----------------------------------------------------------------------
diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/AbstractDistribZkTestBase.java b/solr/test-framework/src/java/org/apache/solr/cloud/AbstractDistribZkTestBase.java
index 7b3617b..4ac6d5a 100644
--- a/solr/test-framework/src/java/org/apache/solr/cloud/AbstractDistribZkTestBase.java
+++ b/solr/test-framework/src/java/org/apache/solr/cloud/AbstractDistribZkTestBase.java
@@ -234,7 +234,7 @@ public abstract class AbstractDistribZkTestBase extends BaseDistributedSearchTes
     fail("Illegal state, was: " + coreState + " expected:" + expectedState + " clusterState:" + reader.getClusterState());
   }
   
-  protected void assertAllActive(String collection,ZkStateReader zkStateReader)
+  protected static void assertAllActive(String collection, ZkStateReader zkStateReader)
       throws KeeperException, InterruptedException {
 
       zkStateReader.forceUpdateCollection(collection);

http://git-wip-us.apache.org/repos/asf/lucene-solr/blob/fd9c4d59/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java
----------------------------------------------------------------------
diff --git a/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java b/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java
index a584dbd..347d0f7 100644
--- a/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java
+++ b/solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java
@@ -336,7 +336,7 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes
 
   }
 
-  protected void waitForCollection(ZkStateReader reader, String collection, int slices) throws Exception {
+  protected static void waitForCollection(ZkStateReader reader, String collection, int slices) throws Exception {
     // wait until shards have started registering...
     int cnt = 30;
     while (!reader.getClusterState().hasCollection(collection)) {