SOLR-5317: SolrCore persistence bugs if defining SolrCores in solr.xml.

git-svn-id: https://svn.apache.org/repos/asf/lucene/dev/trunk@1530856 13f79535-47bb-0310-9956-ffa450edef68

solr/CHANGES.txt

@@ -121,6 +121,9 @@ Bug Fixes
 * SOLR-5306: Extra collection creation parameters like collection.configName are
   not being respected. (Mark Miller, Liang Tianyu, Nathan Neulinger)
 
+* SOLR-5317: SolrCore persistence bugs if defining SolrCores in solr.xml.
+  (Mark Miller, Yago Riveiro)
+
 Security
 ----------------------
 

solr/core/src/java/org/apache/solr/client/solrj/embedded/JettySolrRunner.java

@@ -398,37 +398,40 @@ public class JettySolrRunner {
       startedBefore = true;
     }
     
-    if( dataDir != null) {
+    if (dataDir != null) {
       System.setProperty("solr.data.dir", dataDir);
     }
-    if( solrUlogDir != null) {
+    if (solrUlogDir != null) {
       System.setProperty("solr.ulog.dir", solrUlogDir);
     }
-    if(shards != null) {
+    if (shards != null) {
       System.setProperty("shard", shards);
     }
     if (coreNodeName != null) {
       System.setProperty("coreNodeName", coreNodeName);
     }
-    
-    if (!server.isRunning()) {
-      server.start();
-    }
-    synchronized (JettySolrRunner.this) {
-      int cnt = 0;
-      while (!waitOnSolr) {
-        this.wait(100);
-        if (cnt++ == 5) {
-          throw new RuntimeException("Jetty/Solr unresponsive");
+    try {
+      
+      if (!server.isRunning()) {
+        server.start();
+      }
+      synchronized (JettySolrRunner.this) {
+        int cnt = 0;
+        while (!waitOnSolr) {
+          this.wait(100);
+          if (cnt++ == 5) {
+            throw new RuntimeException("Jetty/Solr unresponsive");
+          }
         }
       }
+    } finally {
+      
+      System.clearProperty("shard");
+      System.clearProperty("solr.data.dir");
+      System.clearProperty("coreNodeName");
+      System.clearProperty("solr.ulog.dir");
     }
     
-    System.clearProperty("shard");
-    System.clearProperty("solr.data.dir");
-    System.clearProperty("coreNodeName");
-    System.clearProperty("solr.ulog.dir");
-
   }
 
   public void stop() throws Exception {

solr/core/src/java/org/apache/solr/core/CoreContainer.java

@@ -17,25 +17,7 @@
 
 package org.apache.solr.core;
 
-import com.google.common.collect.Maps;
-import org.apache.solr.cloud.ZkController;
-import org.apache.solr.cloud.ZkSolrResourceLoader;
-import org.apache.solr.common.SolrException;
-import org.apache.solr.common.SolrException.ErrorCode;
-import org.apache.solr.common.cloud.ZooKeeperException;
-import org.apache.solr.common.util.ExecutorUtil;
-import org.apache.solr.handler.admin.CollectionsHandler;
-import org.apache.solr.handler.admin.CoreAdminHandler;
-import org.apache.solr.handler.admin.InfoHandler;
-import org.apache.solr.handler.component.ShardHandlerFactory;
-import org.apache.solr.logging.LogWatcher;
-import org.apache.solr.schema.IndexSchema;
-import org.apache.solr.schema.IndexSchemaFactory;
-import org.apache.solr.util.DefaultSolrThreadFactory;
-import org.apache.solr.util.FileUtils;
-import org.apache.zookeeper.KeeperException;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+import static com.google.common.base.Preconditions.checkNotNull;
 
 import java.io.File;
 import java.text.SimpleDateFormat;
@@ -57,11 +39,27 @@ import java.util.concurrent.ExecutorCompletionService;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
-import java.util.concurrent.LinkedBlockingQueue;
-import java.util.concurrent.ThreadPoolExecutor;
-import java.util.concurrent.TimeUnit;
 
-import static com.google.common.base.Preconditions.checkNotNull;
+import org.apache.solr.cloud.ZkController;
+import org.apache.solr.cloud.ZkSolrResourceLoader;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
+import org.apache.solr.common.cloud.ZooKeeperException;
+import org.apache.solr.common.util.ExecutorUtil;
+import org.apache.solr.handler.admin.CollectionsHandler;
+import org.apache.solr.handler.admin.CoreAdminHandler;
+import org.apache.solr.handler.admin.InfoHandler;
+import org.apache.solr.handler.component.ShardHandlerFactory;
+import org.apache.solr.logging.LogWatcher;
+import org.apache.solr.schema.IndexSchema;
+import org.apache.solr.schema.IndexSchemaFactory;
+import org.apache.solr.util.DefaultSolrThreadFactory;
+import org.apache.solr.util.FileUtils;
+import org.apache.zookeeper.KeeperException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.collect.Maps;
 
 
 /**

solr/core/src/java/org/apache/solr/core/SolrXMLCoresLocator.java

@@ -19,6 +19,7 @@ package org.apache.solr.core;
 
 import com.google.common.base.Charsets;
 import com.google.common.collect.ImmutableList;
+
 import org.apache.commons.lang.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -28,6 +29,8 @@ import java.io.FileOutputStream;
 import java.io.IOException;
 import java.io.OutputStreamWriter;
 import java.io.Writer;
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
@@ -140,10 +143,13 @@ public class SolrXMLCoresLocator implements CoresLocator {
 
   @Override
   public final void persist(CoreContainer cc, CoreDescriptor... coreDescriptors) {
-    doPersist(buildSolrXML(cc.getCoreDescriptors()));
+    List<CoreDescriptor> cds = new ArrayList<CoreDescriptor>(cc.getCoreDescriptors().size() + coreDescriptors.length);
+    cds.addAll(cc.getCoreDescriptors());
+    cds.addAll(Arrays.asList(coreDescriptors));
+    doPersist(buildSolrXML(cds));
   }
 
-  protected void doPersist(String xml) {
+  protected synchronized void doPersist(String xml) {
     File file = new File(cfg.config.getResourceLoader().getInstanceDir(), ConfigSolr.SOLR_XML_FILE);
     try {
       Writer writer = new OutputStreamWriter(new FileOutputStream(file), Charsets.UTF_8);
@@ -163,12 +169,14 @@ public class SolrXMLCoresLocator implements CoresLocator {
 
   @Override
   public void delete(CoreContainer cc, CoreDescriptor... coreDescriptors) {
-    this.persist(cc, coreDescriptors);
+    // coreDescriptors is kind of a useless param - we persist the current state off cc
+    this.persist(cc);
   }
 
   @Override
   public void rename(CoreContainer cc, CoreDescriptor oldCD, CoreDescriptor newCD) {
-    this.persist(cc, oldCD, newCD);
+    // we don't need those params, we just write out the current cc state
+    this.persist(cc);
   }
 
   @Override

solr/core/src/test-files/solr/solr-no-core-old-style.xml

@@ -0,0 +1,41 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+ Licensed to the Apache Software Foundation (ASF) under one or more
+ contributor license agreements.  See the NOTICE file distributed with
+ this work for additional information regarding copyright ownership.
+ The ASF licenses this file to You under the Apache License, Version 2.0
+ (the "License"); you may not use this file except in compliance with
+ the License.  You may obtain a copy of the License at
+
+     http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+-->
+
+<!--
+ All (relative) paths are relative to the installation path
+  
+  persistent: Save changes made via the API to this file
+  sharedLib: path to a lib directory that will be shared across all cores
+-->
+<solr persistent="${solr.xml.persist:true}">
+
+  <!--
+  adminPath: RequestHandler path to manage cores.  
+    If 'null' (or absent), cores will not be manageable via request handler
+  -->
+  <cores adminPath="/admin/cores" defaultCoreName="collection1" host="127.0.0.1" hostPort="${hostPort:8983}" 
+         hostContext="${hostContext:solr}" zkClientTimeout="${solr.zkclienttimeout:30000}" numShards="${numShards:3}" shareSchema="${shareSchema:false}" 
+         genericCoreNodeNames="${genericCoreNodeNames:true}" leaderVoteWait="0"
+         distribUpdateConnTimeout="${distribUpdateConnTimeout:15000}" distribUpdateSoTimeout="${distribUpdateSoTimeout:120000}">
+    <shardHandlerFactory name="shardHandlerFactory" class="HttpShardHandlerFactory">
+      <int name="socketTimeout">${socketTimeout:120000}</int>
+      <int name="connTimeout">${connTimeout:15000}</int>
+    </shardHandlerFactory>
+  </cores>
+  
+</solr>

solr/core/src/test/org/apache/solr/cloud/CollectionsAPIDistributedZkTest.java

@@ -100,7 +100,10 @@ public class CollectionsAPIDistributedZkTest extends AbstractFullDistribZkTestBa
   
   CompletionService<Request> completionService;
   Set<Future<Request>> pending;
+  
+  // we randomly use a second config set rather than just one
   private boolean secondConfigSet = random().nextBoolean();
+  private boolean oldStyleSolrXml = false;
   
   @BeforeClass
   public static void beforeThisClass2() throws Exception {
@@ -112,6 +115,14 @@ public class CollectionsAPIDistributedZkTest extends AbstractFullDistribZkTestBa
   public void setUp() throws Exception {
     super.setUp();
     
+    useJettyDataDir = false;
+    
+    oldStyleSolrXml = random().nextBoolean();
+    if (oldStyleSolrXml) {
+      System.err.println("Using old style solr.xml");
+    } else {
+      System.err.println("Using new style solr.xml");
+    }
     if (secondConfigSet ) {
       String zkHost = zkServer.getZkHost();
       String zkAddress = zkServer.getZkAddress();
@@ -144,7 +155,8 @@ public class CollectionsAPIDistributedZkTest extends AbstractFullDistribZkTestBa
   }
   
   protected String getSolrXml() {
-    return "solr-no-core.xml";
+    // test old style and new style solr.xml
+    return oldStyleSolrXml ? "solr-no-core-old-style.xml" : "solr-no-core.xml";
   }
 
   
@@ -572,6 +584,27 @@ public class CollectionsAPIDistributedZkTest extends AbstractFullDistribZkTestBa
       // poll for a second - it can take a moment before we are ready to serve
       waitForNon403or404or503(collectionClient);
     }
+    
+    // sometimes we restart one of the jetty nodes
+    if (random().nextBoolean()) {
+      JettySolrRunner jetty = jettys.get(random().nextInt(jettys.size()));
+      ChaosMonkey.stop(jetty);
+      ChaosMonkey.start(jetty);
+      
+      for (Entry<String,List<Integer>> entry : collectionInfosEntrySet) {
+        String collection = entry.getKey();
+        List<Integer> list = entry.getValue();
+        checkForCollection(collection, list, null);
+        
+        String url = getUrlFromZk(collection);
+        
+        HttpSolrServer collectionClient = new HttpSolrServer(url);
+        
+        // poll for a second - it can take a moment before we are ready to serve
+        waitForNon403or404or503(collectionClient);
+      }
+    }
+
     ZkStateReader zkStateReader = getCommonCloudSolrServer().getZkStateReader();
     for (int j = 0; j < cnt; j++) {
       waitForRecoveriesToFinish("awholynewcollection_" + j, zkStateReader, false);
@@ -759,10 +792,12 @@ public class CollectionsAPIDistributedZkTest extends AbstractFullDistribZkTestBa
         .getFilter()).getCores();
     Collection<SolrCore> theCores = cores.getCores();
     for (SolrCore core : theCores) {
-      // look for core props file
-      assertTrue("Could not find expected core.properties file",
-          new File((String) core.getStatistics().get("instanceDir"),
-              "core.properties").exists());
+      if (!oldStyleSolrXml) {
+        // look for core props file
+        assertTrue("Could not find expected core.properties file",
+            new File((String) core.getStatistics().get("instanceDir"),
+                "core.properties").exists());
+      }
       
       assertEquals(
           SolrResourceLoader.normalizeDir(jetty.getSolrHome() + File.separator

solr/test-framework/src/java/org/apache/solr/cloud/AbstractDistribZkTestBase.java

@@ -148,7 +148,7 @@ public abstract class AbstractDistribZkTestBase extends BaseDistributedSearchTes
       for (Map.Entry<String,Slice> entry : slices.entrySet()) {
         Map<String,Replica> shards = entry.getValue().getReplicasMap();
         for (Map.Entry<String,Replica> shard : shards.entrySet()) {
-          if (verbose) System.out.println("rstate:"
+          if (verbose) System.out.println("replica:" + shard.getValue().getName() + " rstate:"
               + shard.getValue().getStr(ZkStateReader.STATE_PROP)
               + " live:"
               + clusterState.liveNodesContain(shard.getValue().getNodeName()));

solr/test-framework/src/java/org/apache/solr/cloud/AbstractFullDistribZkTestBase.java

@@ -121,6 +121,7 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes
   protected Map<String,CloudJettyRunner> shardToLeaderJetty = new HashMap<String,CloudJettyRunner>();
   private boolean cloudInit;
   protected boolean checkCreatedVsState;
+  protected boolean useJettyDataDir = true;
   
   public static class CloudJettyRunner {
     public JettySolrRunner jetty;
@@ -346,8 +347,8 @@ public abstract class AbstractFullDistribZkTestBase extends AbstractDistribZkTes
           getClass().getName() + "-jetty" + cnt + "-" + System.currentTimeMillis());
       jettyDir.mkdirs();
       setupJettySolrHome(jettyDir);
-      JettySolrRunner j = createJetty(jettyDir, getDataDir(testDir + "/jetty"
-          + cnt), null, "solrconfig.xml", null);
+      JettySolrRunner j = createJetty(jettyDir, useJettyDataDir ? getDataDir(testDir + "/jetty"
+          + cnt) : null, null, "solrconfig.xml", null);
       jettys.add(j);
       SolrServer client = createNewSolrServer(j.getLocalPort());
       clients.add(client);