SOLR-5543: Core Swaps result in duplicate entries in solr.xml

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

solr/CHANGES.txt

@@ -74,7 +74,7 @@ Apache ZooKeeper 3.4.5
 Upgrading from Solr 4.6.0
 ----------------------
 
-* CloudSolrServer and LBHttpSolrServer no longer declare MalfurmedURLException
+* CloudSolrServer and LBHttpSolrServer no longer declare MalformedURLException
   as thrown from their constructors.  
                       
 Detailed Change List
@@ -204,6 +204,9 @@ Bug Fixes
 * SOLR-5547: Creating a collection alias using SolrJ's CollectionAdminRequest
   sets the alias name and the collections to alias to the same value.
   (Aaron Schram, Mark Miller)
+
+* SOLR-5543: Core swaps resulted in duplicate core entries in solr.xml when 
+  using solr.xml persistence. (Bill Bell, Alan Woodward)
   
 Optimizations
 ----------------------

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

@@ -17,14 +17,32 @@
 
 package org.apache.solr.core;
 
-import static com.google.common.base.Preconditions.checkNotNull;
+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.update.UpdateShardHandler;
+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 java.io.File;
 import java.text.SimpleDateFormat;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
@@ -41,31 +59,7 @@ import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
 
-import org.apache.http.client.HttpClient;
-import org.apache.http.conn.ClientConnectionManager;
-import org.apache.http.impl.conn.PoolingClientConnectionManager;
-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.common.util.SolrjNamedThreadFactory;
-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.update.UpdateShardHandler;
-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;
+import static com.google.common.base.Preconditions.checkNotNull;
 
 
 /**
@@ -737,7 +731,7 @@ public class CoreContainer {
     n1 = checkDefault(n1);
     solrCores.swap(n0, n1);
 
-    coresLocator.persist(this, solrCores.getCoreDescriptor(n0), solrCores.getCoreDescriptor(n1));
+    coresLocator.swap(this, solrCores.getCoreDescriptor(n0), solrCores.getCoreDescriptor(n1));
 
     log.info("swapped: "+n0 + " with " + n1);
   }

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

@@ -110,6 +110,11 @@ public class CorePropertiesLocator implements CoresLocator {
     persist(cc, newCD);
   }
 
+  @Override
+  public void swap(CoreContainer cc, CoreDescriptor cd1, CoreDescriptor cd2) {
+    persist(cc, cd1, cd2);
+  }
+
   @Override
   public List<CoreDescriptor> discover(CoreContainer cc) {
     logger.info("Looking for core definitions underneath {}", rootDirectory.getAbsolutePath());

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

@@ -55,6 +55,14 @@ public interface CoresLocator {
    */
   public void rename(CoreContainer cc, CoreDescriptor oldCD, CoreDescriptor newCD);
 
+  /**
+   * Swap two core definitions
+   * @param cc  the CoreContainer
+   * @param cd1 the core descriptor of the first core, after swapping
+   * @param cd2 the core descriptor of the second core, after swapping
+   */
+  public void swap(CoreContainer cc, CoreDescriptor cd1, CoreDescriptor cd2);
+
   /**
    * Load all the CoreDescriptors from persistence store
    * @param cc the CoreContainer

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

@@ -19,7 +19,6 @@ package org.apache.solr.core;
 
 import com.google.common.base.Charsets;
 import com.google.common.collect.ImmutableList;
-
 import org.apache.commons.io.IOUtils;
 import org.apache.commons.lang.StringUtils;
 import org.slf4j.Logger;
@@ -32,11 +31,9 @@ import java.io.OutputStreamWriter;
 import java.io.Writer;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Properties;
-import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -190,6 +187,11 @@ public class SolrXMLCoresLocator implements CoresLocator {
     this.persist(cc);
   }
 
+  @Override
+  public void swap(CoreContainer cc, CoreDescriptor cd1, CoreDescriptor cd2) {
+    this.persist(cc);
+  }
+
   @Override
   public List<CoreDescriptor> discover(CoreContainer cc) {
 

solr/core/src/test/org/apache/solr/core/TestSolrXmlPersistence.java

@@ -48,6 +48,8 @@ import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.List;
 
+import static org.hamcrest.core.Is.is;
+
 public class TestSolrXmlPersistence extends SolrTestCaseJ4 {
 
   private File solrHomeDirectory = new File(TEMP_DIR, this.getClass().getName());
@@ -186,8 +188,24 @@ public class TestSolrXmlPersistence extends SolrTestCaseJ4 {
     doTestSwap("SystemVars2", "SystemVars1");
   }
 
+  /*
+  Count the number of times substring appears in target
+   */
+  private int countOccurrences(String target, String substring) {
+    int pos = -1, count = 0;
+    while ((pos = target.indexOf(substring, pos + 1)) != -1) {
+      count++;
+    }
+    return count;
+  }
+
   private void doTestSwap(String from, String to) throws Exception {
     CoreContainer cc = init(SOLR_XML_LOTS_SYSVARS, "SystemVars1", "SystemVars2");
+    SolrXMLCoresLocator.NonPersistingLocator locator
+        = (SolrXMLCoresLocator.NonPersistingLocator) cc.getCoresLocator();
+
+    int coreCount = countOccurrences(locator.xml, "<core ");
+
     try {
       final CoreAdminHandler admin = new CoreAdminHandler(cc);
       SolrQueryResponse resp = new SolrQueryResponse();
@@ -199,6 +217,9 @@ public class TestSolrXmlPersistence extends SolrTestCaseJ4 {
               resp);
       assertNull("Exception on swap", resp.getException());
 
+      assertThat("Swapping cores should leave the same number of cores as before",
+          countOccurrences(locator.xml, "<core "), is(coreCount));
+
       String[] persistList = getAllNodes();
       String[] expressions = new String[persistList.length];