Changes for SOLR-4662, finalizing the new form of solrxml

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

solr/CHANGES.txt

@@ -111,9 +111,6 @@ New Features
   there are a large number of entries in the multiValued field. Conspicuously, this will
   prevent the "best" match from being found if it appears later in the MV list than the
   cutoff specified by either of these params. (Erick Erickson)
-  
-* SOLR-4663: Complain loudly and do NOT load cores if they have the same data dir or the 
-  same name
 
 * SOLR-4675: Improve PostingsSolrHighlighter to support per-field/query-time overrides
   and add additional configuration parameters. See the javadocs for more details and
@@ -136,6 +133,16 @@ New Features
   Also fixed a problem whereby properties like solr.solr.datadir would be persisted
   to solr.xml. Also, cores that didn't happen to be loaded were not persisted. 
   (Erick Erickson)
+  
+* SOLR-4662: Finalize what we're going to do with solr.xml, auto-discovery, config sets.
+  The format of solr.xml is changing. <core> and <cores> tags are no longer supported as
+  of 5.0 (but will be supported for 4.x). defaultCoreName, persistent and adminPath are
+  obsolete in 5.0 as well. Discovery mode will be the way cores are enumerated in 5.0.
+  Supports a new value (<cores> attribute coreRootDirectory in 4.x, element in <solr> in
+  5.0) for basing the enumeration of cores. In the new way of doing things, a
+  core.propeties file will mark the instanceDir for that core, and instanceDir will
+  be obsolete as well
+   
 
 Bug Fixes
 ----------------------

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

@@ -37,18 +37,48 @@ import java.util.Properties;
  */
 public interface ConfigSolr {
 
-  public static enum ConfLevel {
+  // Ugly for now, but we'll at least be able to centralize all of the differences between 4x and 5x.
-    SOLR, SOLR_CORES, SOLR_CORES_CORE, SOLR_LOGGING, SOLR_LOGGING_WATCHER
+  public static enum CfgProp {
+    SOLR_ADMINHANDLER,
+    SOLR_CORELOADTHREADS,
+    SOLR_COREROOTDIRECTORY,
+    SOLR_DISTRIBUPDATECONNTIMEOUT,
+    SOLR_DISTRIBUPDATESOTIMEOUT,
+    SOLR_HOST,
+    SOLR_HOSTCONTEXT,
+    SOLR_HOSTPORT,
+    SOLR_LEADERVOTEWAIT,
+    SOLR_LOGGING_CLASS,
+    SOLR_LOGGING_ENABLED,
+    SOLR_LOGGING_WATCHER_SIZE,
+    SOLR_LOGGING_WATCHER_THRESHOLD,
+    SOLR_MANAGEMENTPATH,
+    SOLR_SHAREDLIB,
+    SOLR_SHARDHANDLERFACTORY_CLASS,
+    SOLR_SHARDHANDLERFACTORY_CONNTIMEOUT,
+    SOLR_SHARDHANDLERFACTORY_NAME,
+    SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT,
+    SOLR_SHARESCHEMA,
+    SOLR_TRANSIENTCACHESIZE,
+    SOLR_ZKCLIENTTIMEOUT,
+    SOLR_ZKHOST,
+
+    //TODO: Remove all of these elements for 5.0
+    SOLR_PERSISTENT,
+    SOLR_CORES_DEFAULT_CORE_NAME,
+    SOLR_ADMINPATH
   };
 
   public final static String CORE_PROP_FILE = "core.properties";
   public final static String SOLR_XML_FILE = "solr.xml";
 
-  public int getInt(ConfLevel level, String tag, int def);
+  public int getInt(CfgProp prop, int def);
 
-  public boolean getBool(ConfLevel level, String tag, boolean defValue);
+  public boolean getBool(CfgProp prop,boolean defValue);
 
-  public String get(ConfLevel level, String tag, String def);
+  public String get(CfgProp prop, String def);
+
+  public String getOrigProp(CfgProp prop, String def);
 
   public void substituteProperties();
 
@@ -79,4 +109,6 @@ public interface ConfigSolr {
   // If the core is not to be loaded (say two cores defined with the same name or with the same data dir), return
   // the reason. If it's OK to load the core, return null.
   public String getBadCoreMessage(String name);
+
+  public boolean is50OrLater();
 }

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

@@ -62,8 +62,8 @@ import java.util.Set;
 /**
  * ConfigSolrXml
  * <p/>
- * This class is entirely to localize the backwards compatibility for dealing with specific issues when transitioning
+ * This class is entirely to localize the dealing with specific issues when transitioning from old-style solr.xml
- * from solr.xml to a solr.properties-based, enumeration/discovery of defined cores. See SOLR-4196 for background.
+ * to new-style xml, enumeration/discovery of defined cores. See SOLR-4196 for background.
  * <p/>
  * @since solr 4.3
  *
@@ -73,45 +73,181 @@ import java.util.Set;
  *
  */
 
+
 public class ConfigSolrXml extends Config implements ConfigSolr {
 
-  private static Map<ConfLevel, String> prefixes;
+  private boolean is50OrLater = false;
-  private boolean isAutoDiscover = false;
 
   private final Map<String, CoreDescriptorPlus> coreDescriptorPlusMap = new HashMap<String, CoreDescriptorPlus>();
   private NodeList coreNodes = null;
   private final Map<String, String> badCores = new HashMap<String, String>();
     // List of cores that we should _never_ load. Ones with dup names or duplicate datadirs or...
 
-  static {
-    prefixes = new HashMap<ConfLevel, String>();
 
-    prefixes.put(ConfLevel.SOLR, "solr/@");
+  private Map<CfgProp, String> propMap = new HashMap<CfgProp, String>();
-    prefixes.put(ConfLevel.SOLR_CORES, "solr/cores/@");
-    prefixes.put(ConfLevel.SOLR_CORES_CORE, "solr/cores/core/@");
-    prefixes.put(ConfLevel.SOLR_LOGGING, "solr/logging/@");
-    prefixes.put(ConfLevel.SOLR_LOGGING_WATCHER, "solr/logging/watcher/@");
-  }
 
   public ConfigSolrXml(SolrResourceLoader loader, String name, InputStream is, String prefix,
-                       boolean subProps, CoreContainer container) throws ParserConfigurationException, IOException, SAXException {
+                       boolean subProps, CoreContainer container)
+      throws ParserConfigurationException, IOException, SAXException {
+
     super(loader, name, new InputSource(is), prefix, subProps);
-    initCoreList(container);
+    init(container);
   }
 
 
-  public ConfigSolrXml(SolrResourceLoader loader, Config cfg, CoreContainer container) throws TransformerException, IOException {
+  public ConfigSolrXml(SolrResourceLoader loader, Config cfg, CoreContainer container)
+      throws TransformerException, IOException {
+
     super(loader, null, copyDoc(cfg.getDocument())); // Mimics a call from CoreContainer.
+    init(container);
+  }
+  private void init(CoreContainer container) throws IOException {
+    is50OrLater = getNode("solr/cores", false) == null;
+
+    // Do sanity checks, old and new style. Pretty exhaustive for now, but want to hammer this.
+    // TODO: 5.0 maybe remove this checking, it's mostly for correctness as we make this transition.
+
+    if (is50OrLater()) {
+      insureFail("solr/@coreLoadThreads");
+      insureFail("solr/@persist");
+      insureFail("solr/@sharedLib");
+      insureFail("solr/@zkHost");
+
+      insureFail("solr/logging/@class");
+      insureFail("solr/logging/@enabled");
+      insureFail("solr/logging/watcher/@size");
+      insureFail("solr/logging/watcher/@threshold");
+
+      insureFail("solr/cores/@adminHandler");
+      insureFail("solr/cores/@distribUpdateConnTimeout");
+      insureFail("solr/cores/@distribUpdateSoTimeout");
+      insureFail("solr/cores/@host");
+      insureFail("solr/cores/@hostContext");
+      insureFail("solr/cores/@hostPort");
+      insureFail("solr/cores/@leaderVoteWait");
+      insureFail("solr/cores/@managementPath");
+      insureFail("solr/cores/@shareSchema");
+      insureFail("solr/cores/@transientCacheSize");
+      insureFail("solr/cores/@zkClientTimeout");
+
+      // These have no counterpart in 5.0, asking for any o fthese in Solr 5.0 will result in an error being
+      // thrown.
+      insureFail("solr/cores/@defaultCoreName");
+      insureFail("solr/@persistent");
+      insureFail("solr/cores/@adminPath");
+    } else {
+      insureFail("solr/str[@name='adminHandler']");
+      insureFail("solr/int[@name='coreLoadThreads']");
+      insureFail("solr/str[@name='coreRootDirectory']");
+      insureFail("solr/solrcloud/int[@name='distribUpdateConnTimeout']");
+      insureFail("solr/solrcloud/int[@name='distribUpdateSoTimeout']");
+      insureFail("solr/solrcloud/str[@name='host']");
+      insureFail("solr/solrcloud/str[@name='hostContext']");
+      insureFail("solr/solrcloud/int[@name='hostPort']");
+      insureFail("solr/solrcloud/int[@name='leaderVoteWait']");
+      insureFail("solr/str[@name='managementPath']");
+      insureFail("solr/str[@name='sharedLib']");
+      insureFail("solr/str[@name='shareSchema']");
+      insureFail("solr/int[@name='transientCacheSize']");
+      insureFail("solr/solrcloud/int[@name='zkClientTimeout']");
+      insureFail("solr/solrcloud/int[@name='zkHost']");
+
+      insureFail("solr/logging/str[@name='class']");
+      insureFail("solr/logging/str[@name='enabled']");
+
+      insureFail("solr/logging/watcher/int[@name='size']");
+      insureFail("solr/logging/watcher/int[@name='threshold']");
+
+    }
+    fillPropMap();
     initCoreList(container);
   }
+  private void insureFail(String xPath) {
 
+    if (getVal(xPath, false) != null) {
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Should not have found " + xPath +
+          " solr.xml may be a mix of old and new style formats.");
+    }
+  }
+
+  // We can do this in 5.0 when we read the solr.xml since we don't need to keep the original around for persistence.
+  private String doSub(String path) {
+    String val = getVal(path, false);
+    if (val != null) {
+      val = PropertiesUtil.substituteProperty(val, null);
+    }
+    return val;
+  }
+  private void fillPropMap() {
+    if (is50OrLater) { // Can do the prop subs early here since we don't need to preserve them for persistence.
+      propMap.put(CfgProp.SOLR_ADMINHANDLER, doSub("solr/str[@name='adminHandler']"));
+      propMap.put(CfgProp.SOLR_CORELOADTHREADS, doSub("solr/int[@name='coreLoadThreads']"));
+      propMap.put(CfgProp.SOLR_COREROOTDIRECTORY, doSub("solr/str[@name='coreRootDirectory']"));
+      propMap.put(CfgProp.SOLR_DISTRIBUPDATECONNTIMEOUT, doSub("solr/solrcloud/int[@name='distribUpdateConnTimeout']"));
+      propMap.put(CfgProp.SOLR_DISTRIBUPDATESOTIMEOUT, doSub("solr/solrcloud/int[@name='distribUpdateSoTimeout']"));
+      propMap.put(CfgProp.SOLR_HOST, doSub("solr/solrcloud/str[@name='host']"));
+      propMap.put(CfgProp.SOLR_HOSTCONTEXT, doSub("solr/solrcloud/str[@name='hostContext']"));
+      propMap.put(CfgProp.SOLR_HOSTPORT, doSub("solr/solrcloud/int[@name='hostPort']"));
+      propMap.put(CfgProp.SOLR_LEADERVOTEWAIT, doSub("solr/solrcloud/int[@name='leaderVoteWait']"));
+      propMap.put(CfgProp.SOLR_MANAGEMENTPATH, doSub("solr/str[@name='managementPath']"));
+      propMap.put(CfgProp.SOLR_SHAREDLIB, doSub("solr/str[@name='sharedLib']"));
+      propMap.put(CfgProp.SOLR_SHARESCHEMA, doSub("solr/str[@name='shareSchema']"));
+      propMap.put(CfgProp.SOLR_TRANSIENTCACHESIZE, doSub("solr/int[@name='transientCacheSize']"));
+      propMap.put(CfgProp.SOLR_ZKCLIENTTIMEOUT, doSub("solr/solrcloud/int[@name='zkClientTimeout']"));
+      propMap.put(CfgProp.SOLR_ZKHOST, doSub("solr/solrcloud/str[@name='zkHost']"));
+
+      propMap.put(CfgProp.SOLR_LOGGING_CLASS, doSub("solr/logging/str[@name='class']"));
+      propMap.put(CfgProp.SOLR_LOGGING_ENABLED, doSub("solr/logging/str[@name='enabled']"));
+
+      propMap.put(CfgProp.SOLR_LOGGING_WATCHER_SIZE, doSub("solr/logging/watcher/int[@name='size']"));
+      propMap.put(CfgProp.SOLR_LOGGING_WATCHER_THRESHOLD, doSub("solr/logging/watcher/int[@name='threshold']"));
+      propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS, doSub("solr/shardHandlerFactory/@class"));
+      propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_NAME, doSub("solr/shardHandlerFactory/@name"));
+      propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CONNTIMEOUT, doSub("solr/shardHandlerFactory/int[@name='connTimeout']"));
+      propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT, doSub("solr/shardHandlerFactory/int[@name='socketTimeout']"));
+    } else {
+      propMap.put(CfgProp.SOLR_CORELOADTHREADS, getVal("solr/@coreLoadThreads", false));
+      propMap.put(CfgProp.SOLR_SHAREDLIB, getVal("solr/@sharedLib", false));
+      propMap.put(CfgProp.SOLR_ZKHOST, getVal("solr/@zkHost", false));
+
+      propMap.put(CfgProp.SOLR_LOGGING_CLASS, getVal("solr/logging/@class", false));
+      propMap.put(CfgProp.SOLR_LOGGING_ENABLED, getVal("solr/logging/@enabled", false));
+      propMap.put(CfgProp.SOLR_LOGGING_WATCHER_SIZE, getVal("solr/logging/watcher/@size", false));
+      propMap.put(CfgProp.SOLR_LOGGING_WATCHER_THRESHOLD, getVal("solr/logging/watcher/@threshold", false));
+
+      propMap.put(CfgProp.SOLR_ADMINHANDLER, getVal("solr/cores/@adminHandler", false));
+      propMap.put(CfgProp.SOLR_DISTRIBUPDATECONNTIMEOUT, getVal("solr/cores/@distribUpdateConnTimeout", false));
+      propMap.put(CfgProp.SOLR_DISTRIBUPDATESOTIMEOUT, getVal("solr/cores/@distribUpdateSoTimeout", false));
+      propMap.put(CfgProp.SOLR_HOST, getVal("solr/cores/@host", false));
+      propMap.put(CfgProp.SOLR_HOSTCONTEXT, getVal("solr/cores/@hostContext", false));
+      propMap.put(CfgProp.SOLR_HOSTPORT, getVal("solr/cores/@hostPort", false));
+      propMap.put(CfgProp.SOLR_LEADERVOTEWAIT, getVal("solr/cores/@leaderVoteWait", false));
+      propMap.put(CfgProp.SOLR_MANAGEMENTPATH, getVal("solr/cores/@managementPath", false));
+      propMap.put(CfgProp.SOLR_SHARESCHEMA, getVal("solr/cores/@shareSchema", false));
+      propMap.put(CfgProp.SOLR_TRANSIENTCACHESIZE, getVal("solr/cores/@transientCacheSize", false));
+      propMap.put(CfgProp.SOLR_ZKCLIENTTIMEOUT, getVal("solr/cores/@zkClientTimeout", false));
+      propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS, getVal("solr/shardHandlerFactory/@class", false));
+      propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_NAME, getVal("solr/shardHandlerFactory/@name", false));
+      propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_CONNTIMEOUT, getVal(  "solr/shardHandlerFactory/int[@connTimeout]", false));
+      propMap.put(CfgProp.SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT, getVal("solr/shardHandlerFactory/int[@socketTimeout]", false));
+
+      // These have no counterpart in 5.0, asking, for any o, fthese in Solr 5.0 will result in an error being
+      // thrown.
+      propMap.put(CfgProp.SOLR_CORES_DEFAULT_CORE_NAME, getVal("solr/cores/@defaultCoreName", false));
+      propMap.put(CfgProp.SOLR_PERSISTENT, getVal("solr/@persistent", false));
+      propMap.put(CfgProp.SOLR_ADMINPATH, getVal("solr/cores/@adminPath", false));
+    }
+  }
+
+  //NOTE:
   public void initCoreList(CoreContainer container) throws IOException {
-    isAutoDiscover = getBool(ConfigSolr.ConfLevel.SOLR_CORES, "autoDiscoverCores", false);
+    if (is50OrLater) {
-    if (isAutoDiscover) {
+      if (container != null) { //TODO: 5.0. Yet another bit of nonsense only because of the test harness.
-      synchronized (coreDescriptorPlusMap) {
+        synchronized (coreDescriptorPlusMap) {
-        walkFromHere(new File(container.getSolrHome()), container, new HashMap<String, CoreDescriptorPlus>());
+          String coreRoot = get(CfgProp.SOLR_COREROOTDIRECTORY, container.getSolrHome());
+          walkFromHere(new File(coreRoot), container, new HashMap<String, String>(), new HashMap<String, String>());
+        }
       }
-
     } else {
       coreNodes = (NodeList) evaluate("solr/cores/core",
           XPathConstants.NODESET);
@@ -160,21 +296,37 @@ public class ConfigSolrXml extends Config implements ConfigSolr {
     return (Document) result.getNode();
   }
 
+  //TODO: For 5.0, you shouldn't have to do the sbustituteProperty, this is anothe bit
+  // of awkward back-compat due to persistence.
   @Override
-  public int getInt(ConfLevel level, String tag, int def) {
+  public int getInt(CfgProp prop, int def) {
-    return getInt(prefixes.get(level) + tag, def);
+    String val = propMap.get(prop);
+    if (val != null) val = PropertiesUtil.substituteProperty(val, null);
+    return (val == null) ? def : Integer.parseInt(val);
   }
 
   @Override
-  public boolean getBool(ConfLevel level, String tag, boolean defValue) {
+  public boolean getBool(CfgProp prop, boolean defValue) {
-    return getBool(prefixes.get(level) + tag, defValue);
+    String val = propMap.get(prop);
+    if (val != null) val = PropertiesUtil.substituteProperty(val, null);
+    return (val == null) ? defValue : Boolean.parseBoolean(val);
   }
 
   @Override
-  public String get(ConfLevel level, String tag, String def) {
+  public String get(CfgProp prop, String def) {
-    return get(prefixes.get(level) + tag, def);
+    String val = propMap.get(prop);
+    if (val != null) val = PropertiesUtil.substituteProperty(val, null);
+    return (val == null) ? def : val;
   }
 
+  // For saving the original property, ${} syntax and all.
+  @Override
+  public String getOrigProp(CfgProp prop, String def) {
+    String val = propMap.get(prop);
+    return (val == null) ? def : val;
+  }
+
+
   public ShardHandlerFactory initShardHandler() {
     PluginInfo info = null;
     Node shfn = getNode("solr/cores/shardHandlerFactory", false);
@@ -227,7 +379,7 @@ public class ConfigSolrXml extends Config implements ConfigSolr {
   public Map<String, String> readCoreAttributes(String coreName) {
     Map<String, String> attrs = new HashMap<String, String>();
 
-    if (isAutoDiscover) {
+    if (is50OrLater) {
       return attrs; // this is a no-op.... intentionally
     }
     synchronized (coreNodes) {
@@ -257,65 +409,100 @@ public class ConfigSolrXml extends Config implements ConfigSolr {
   // deeper in the tree.
   //
   // @param file - the directory we're to either read the properties file from or recurse into.
-  private void walkFromHere(File file, CoreContainer container, Map<String, CoreDescriptorPlus> checkMap) throws IOException {
+  private void walkFromHere(File file, CoreContainer container, Map<String, String>seenDirs, HashMap<String, String> seenCores)
+      throws IOException {
     log.info("Looking for cores in " + file.getAbsolutePath());
+    if (! file.exists()) return;
+
     for (File childFile : file.listFiles()) {
       // This is a little tricky, we are asking if core.properties exists in a child directory of the directory passed
       // in. In other words we're looking for core.properties in the grandchild directories of the parameter passed
       // in. That allows us to gracefully top recursing deep but continue looking wide.
       File propFile = new File(childFile, CORE_PROP_FILE);
       if (propFile.exists()) { // Stop looking after processing this file!
-        log.info("Discovered properties file {}, adding to cores", propFile.getAbsolutePath());
+        addCore(container, seenDirs, seenCores, childFile, propFile);
-        Properties propsOrig = new Properties();
+        continue; // Go on to the sibling directory, don't descend any deeper.
-        InputStream is = new FileInputStream(propFile);
-        try {
-          propsOrig.load(is);
-        } finally {
-          IOUtils.closeQuietly(is);
-        }
-
-        Properties props = new Properties();
-        for (String prop : propsOrig.stringPropertyNames()) {
-          props.put(prop, PropertiesUtil.substituteProperty(propsOrig.getProperty(prop), null));
-        }
-
-        if (props.getProperty(CoreDescriptor.CORE_INSTDIR) == null) {
-          props.setProperty(CoreDescriptor.CORE_INSTDIR, childFile.getPath());
-        }
-
-        if (props.getProperty(CoreDescriptor.CORE_NAME) == null) {
-          // Should default to this directory
-          props.setProperty(CoreDescriptor.CORE_NAME, childFile.getName());
-        }
-        CoreDescriptor desc = new CoreDescriptor(container, props);
-        CoreDescriptorPlus plus = new CoreDescriptorPlus(propFile.getAbsolutePath(), desc, propsOrig);
-        CoreDescriptorPlus check = coreDescriptorPlusMap.get(desc.getName());
-        if (check == null) { // It's bad to have two cores with the same name
-          coreDescriptorPlusMap.put(desc.getName(), plus);
-        } else {
-          String msg = String.format(Locale.ROOT, "More than one core defined for core named %s, paths are '%s' and '%s' ",
-              desc.getName(), check.getFilePath(), plus.getFilePath());
-          log.error(msg);
-          badCores.put(desc.getName(), msg);
-        }
-        check = coreDescriptorPlusMap.get(plus.getCoreDescriptor().getDataDir());
-        if (check == null) {
-          coreDescriptorPlusMap.put(desc.getName(), plus);
-        } else {
-          String msg = String.format(Locale.ROOT, "More than one core points to data dir %s. They are in %s and %s",
-              plus.getCoreDescriptor().getDataDir(), plus.getFilePath(), check.getFilePath());
-          log.error(msg);
-          badCores.put(plus.getCoreDescriptor().getName(), msg);
-        }
-
-        continue; // Go on to the sibling directory
       }
       if (childFile.isDirectory()) {
-        walkFromHere(childFile, container, checkMap);
+        walkFromHere(childFile, container, seenDirs, seenCores);
       }
     }
   }
 
+  private void addCore(CoreContainer container, Map<String, String> seenDirs, Map<String, String> seenCores,
+                       File childFile, File propFile) throws IOException {
+    log.info("Discovered properties file {}, adding to cores", propFile.getAbsolutePath());
+    Properties propsOrig = new Properties();
+    InputStream is = new FileInputStream(propFile);
+    try {
+      propsOrig.load(is);
+    } finally {
+      IOUtils.closeQuietly(is);
+    }
+
+    Properties props = new Properties();
+    for (String prop : propsOrig.stringPropertyNames()) {
+      props.put(prop, PropertiesUtil.substituteProperty(propsOrig.getProperty(prop), null));
+    }
+
+    // Too much of the code depends on this value being here, but it is NOT supported in discovery mode, so
+    // ignore it if present in the core.properties file.
+    props.setProperty(CoreDescriptor.CORE_INSTDIR, childFile.getPath());
+
+    if (props.getProperty(CoreDescriptor.CORE_NAME) == null) {
+      // Should default to this directory
+      props.setProperty(CoreDescriptor.CORE_NAME, childFile.getName());
+    }
+    CoreDescriptor desc = new CoreDescriptor(container, props);
+    CoreDescriptorPlus plus = new CoreDescriptorPlus(propFile.getAbsolutePath(), desc, propsOrig);
+
+    // It's bad to have two cores with the same name or same data dir.
+    if (! seenCores.containsKey(desc.getName()) && ! seenDirs.containsKey(desc.getAbsoluteDataDir())) {
+      coreDescriptorPlusMap.put(desc.getName(), plus);
+      // Use the full path to the prop file so we can unambiguously report the place the error is.
+      seenCores.put(desc.getName(), propFile.getAbsolutePath());
+      seenDirs.put(desc.getAbsoluteDataDir(), propFile.getAbsolutePath());
+      return;
+    }
+
+    // record the appropriate error
+    if (seenCores.containsKey(desc.getName())) {
+      String msg = String.format(Locale.ROOT, "More than one core defined for core named '%s', paths are '%s' and '%s'  Removing both cores.",
+          desc.getName(), propFile.getAbsolutePath(), seenCores.get(desc.getName()));
+      log.error(msg);
+      // Load up as many errors as there are.
+      if (badCores.containsKey(desc.getName())) msg += " " + badCores.get(desc.getName());
+      badCores.put(desc.getName(), msg);
+    }
+    // There's no reason both errors may not have occurred.
+    if (seenDirs.containsKey(desc.getAbsoluteDataDir())) {
+      String msg = String.format(Locale.ROOT, "More than one core points to data dir '%s'. They are in '%s' and '%s'. Removing all offending cores.",
+          desc.getAbsoluteDataDir(), propFile.getAbsolutePath(), seenDirs.get(desc.getAbsoluteDataDir()));
+      if (badCores.containsKey(desc.getName())) msg += " " + badCores.get(desc.getName());
+      log.error(msg);
+      badCores.put(desc.getName(), msg);
+
+      // find the core with this datadir and remove it
+      List<String> badNames = new ArrayList<String>();
+      for (Map.Entry<String, CoreDescriptorPlus> ent : coreDescriptorPlusMap.entrySet()) {
+        if (ent.getValue().getCoreDescriptor().getAbsoluteDataDir().equals(desc.getAbsoluteDataDir())) {
+          badNames.add(ent.getKey());
+          if (! badCores.containsKey(ent.getKey())) {
+            // Record that the first core is also a bad core.
+            badCores.put(ent.getKey(), msg);
+            log.error(msg);
+
+          }
+          break;
+        }
+      }
+      for (String badName : badNames) {
+        coreDescriptorPlusMap.remove(badName);
+      }
+    }
+    coreDescriptorPlusMap.remove(desc.getName());
+  }
+
   public IndexSchema getSchemaFromZk(ZkController zkController, String zkConfigName, String schemaName,
                                      SolrConfig config)
       throws KeeperException, InterruptedException {
@@ -383,7 +570,7 @@ public class ConfigSolrXml extends Config implements ConfigSolr {
   @Override
   public String getCoreNameFromOrig(String origCoreName, SolrResourceLoader loader, String coreName) {
 
-    if (isAutoDiscover) {
+    if (is50OrLater) {
       // first look for an exact match
       for (Map.Entry<String, CoreDescriptorPlus> ent : coreDescriptorPlusMap.entrySet()) {
 
@@ -445,7 +632,7 @@ public class ConfigSolrXml extends Config implements ConfigSolr {
   @Override
   public List<String> getAllCoreNames() {
     List<String> ret = new ArrayList<String>();
-    if (isAutoDiscover) {
+    if (is50OrLater) {
       ret = new ArrayList<String>(coreDescriptorPlusMap.keySet());
     } else {
       synchronized (coreNodes) {
@@ -460,7 +647,7 @@ public class ConfigSolrXml extends Config implements ConfigSolr {
 
   @Override
   public String getProperty(String coreName, String property, String defaultVal) {
-    if (isAutoDiscover) {
+    if (is50OrLater) {
       CoreDescriptorPlus plus = coreDescriptorPlusMap.get(coreName);
       if (plus == null) return defaultVal;
       CoreDescriptor desc = plus.getCoreDescriptor();
@@ -481,7 +668,7 @@ public class ConfigSolrXml extends Config implements ConfigSolr {
 
   @Override
   public Properties readCoreProperties(String coreName) {
-    if (isAutoDiscover) {
+    if (is50OrLater) {
       CoreDescriptorPlus plus = coreDescriptorPlusMap.get(coreName);
       if (plus == null) return null;
       return new Properties(plus.getCoreDescriptor().getCoreProperties());
@@ -502,6 +689,9 @@ public class ConfigSolrXml extends Config implements ConfigSolr {
     return null;
   }
 
+  @Override
+  public boolean is50OrLater() { return is50OrLater; }
+
   static Properties getCoreProperties(String instanceDir, CoreDescriptor dcore) {
     String file = dcore.getPropertiesName();
     if (file == null) file = "conf" + File.separator + "solrcore.properties";
@@ -560,7 +750,7 @@ public class ConfigSolrXml extends Config implements ConfigSolr {
 class CoreDescriptorPlus {
   private CoreDescriptor coreDescriptor;
   private String filePath;
-  private Properties propsOrig;
+  private Properties propsOrig; // TODO: 5.0. Remove this since it's only really used for persisting.
 
   CoreDescriptorPlus(String filePath, CoreDescriptor descriptor, Properties propsOrig) {
     coreDescriptor = descriptor;

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

@@ -382,7 +382,7 @@ public class CoreContainer
 
     ConfigSolr cfg;
     
-    // keep orig config for persist to consult
+    // keep orig config for persist to consult. TODO: Remove this silly stuff for 5.0, persistence not supported.
     try {
       cfg = new ConfigSolrXml(loader, null, is, null, false, this);
       this.cfg = new ConfigSolrXml(loader, (ConfigSolrXml) cfg, this);
@@ -394,7 +394,7 @@ public class CoreContainer
     cfg.substituteProperties();
 
     // add the sharedLib to the shared resource loader before initializing cfg based plugins
-    libDir = cfg.get(ConfigSolr.ConfLevel.SOLR, "sharedLib", null);
+    libDir = cfg.get(ConfigSolr.CfgProp.SOLR_SHAREDLIB , null);
     if (libDir != null) {
       File f = FileUtils.resolvePath(new File(dir), libDir);
       log.info("loading shared library: " + f.getAbsolutePath());
@@ -407,9 +407,9 @@ public class CoreContainer
     coreMaps.allocateLazyCores(cfg, loader);
 
     // Initialize Logging
-    if (cfg.getBool(ConfigSolr.ConfLevel.SOLR_LOGGING, "enabled", true)) {
+    if (cfg.getBool(ConfigSolr.CfgProp.SOLR_LOGGING_ENABLED, true)) {
       String slf4jImpl = null;
-      String fname = cfg.get(ConfigSolr.ConfLevel.SOLR_LOGGING, "class", null);
+      String fname = cfg.get(ConfigSolr.CfgProp.SOLR_LOGGING_CLASS, null);
       try {
         slf4jImpl = StaticLoggerBinder.getSingleton()
             .getLoggerFactoryClassStr();
@@ -441,8 +441,8 @@ public class CoreContainer
         
         if (logging != null) {
           ListenerConfig v = new ListenerConfig();
-          v.size = cfg.getInt(ConfigSolr.ConfLevel.SOLR_LOGGING_WATCHER, "size", 50);
+          v.size = cfg.getInt(ConfigSolr.CfgProp.SOLR_LOGGING_WATCHER_SIZE, 50);
-          v.threshold = cfg.get(ConfigSolr.ConfLevel.SOLR_LOGGING_WATCHER, "threshold", null);
+          v.threshold = cfg.get(ConfigSolr.CfgProp.SOLR_LOGGING_WATCHER_THRESHOLD, null);
           if (v.size > 0) {
             log.info("Registering Log Listener");
             logging.registerListener(v, this);
@@ -450,38 +450,43 @@ public class CoreContainer
         }
       }
     }
-    
+
-    String dcoreName = cfg.get(ConfigSolr.ConfLevel.SOLR_CORES, "defaultCoreName", null);
+
-    if (dcoreName != null && !dcoreName.isEmpty()) {
+    if (! cfg.is50OrLater()) { //TODO: Remove for 5.0
-      defaultCoreName = dcoreName;
+      String dcoreName = cfg.get(ConfigSolr.CfgProp.SOLR_CORES_DEFAULT_CORE_NAME, null);
+      if (dcoreName != null && !dcoreName.isEmpty()) {
+        defaultCoreName = dcoreName;
+      }
+      persistent = cfg.getBool(ConfigSolr.CfgProp.SOLR_PERSISTENT, false);
+      adminPath = cfg.get(ConfigSolr.CfgProp.SOLR_ADMINPATH, null);
     }
-    persistent = cfg.getBool(ConfigSolr.ConfLevel.SOLR, "persistent", false);
+    zkHost = cfg.get(ConfigSolr.CfgProp.SOLR_ZKHOST, null);
-    zkHost = cfg.get(ConfigSolr.ConfLevel.SOLR, "zkHost", null);
+    coreLoadThreads = cfg.getInt(ConfigSolr.CfgProp.SOLR_CORELOADTHREADS, CORE_LOAD_THREADS);
-    coreLoadThreads = cfg.getInt(ConfigSolr.ConfLevel.SOLR, "coreLoadThreads", CORE_LOAD_THREADS);
     
-    adminPath = cfg.get(ConfigSolr.ConfLevel.SOLR_CORES, "adminPath", null);
+
-    shareSchema = cfg.getBool(ConfigSolr.ConfLevel.SOLR_CORES, "shareSchema", DEFAULT_SHARE_SCHEMA);
+    shareSchema = cfg.getBool(ConfigSolr.CfgProp.SOLR_SHARESCHEMA, DEFAULT_SHARE_SCHEMA);
-    zkClientTimeout = cfg.getInt(ConfigSolr.ConfLevel.SOLR_CORES, "zkClientTimeout", DEFAULT_ZK_CLIENT_TIMEOUT);
+    zkClientTimeout = cfg.getInt(ConfigSolr.CfgProp.SOLR_ZKCLIENTTIMEOUT, DEFAULT_ZK_CLIENT_TIMEOUT);
     
-    distribUpdateConnTimeout = cfg.getInt(ConfigSolr.ConfLevel.SOLR_CORES, "distribUpdateConnTimeout", 0);
+    distribUpdateConnTimeout = cfg.getInt(ConfigSolr.CfgProp.SOLR_DISTRIBUPDATECONNTIMEOUT, 0);
-    distribUpdateSoTimeout = cfg.getInt(ConfigSolr.ConfLevel.SOLR_CORES, "distribUpdateSoTimeout", 0);
+    distribUpdateSoTimeout = cfg.getInt(ConfigSolr.CfgProp.SOLR_DISTRIBUPDATESOTIMEOUT, 0);
 
     // Note: initZooKeeper will apply hardcoded default if cloud mode
-    hostPort = cfg.get(ConfigSolr.ConfLevel.SOLR_CORES, "hostPort", null);
+    hostPort = cfg.get(ConfigSolr.CfgProp.SOLR_HOSTPORT, null);
     // Note: initZooKeeper will apply hardcoded default if cloud mode
-    hostContext = cfg.get(ConfigSolr.ConfLevel.SOLR_CORES, "hostContext", null);
+    hostContext = cfg.get(ConfigSolr.CfgProp.SOLR_HOSTCONTEXT, null);
 
-    host = cfg.get(ConfigSolr.ConfLevel.SOLR_CORES, "host", null);
+    host = cfg.get(ConfigSolr.CfgProp.SOLR_HOST, null);
     
-    leaderVoteWait = cfg.get(ConfigSolr.ConfLevel.SOLR_CORES, "leaderVoteWait", LEADER_VOTE_WAIT);
+    leaderVoteWait = cfg.get(ConfigSolr.CfgProp.SOLR_LEADERVOTEWAIT, LEADER_VOTE_WAIT);
+
+    adminHandler = cfg.get(ConfigSolr.CfgProp.SOLR_ADMINHANDLER, null);
+    managementPath = cfg.get(ConfigSolr.CfgProp.SOLR_MANAGEMENTPATH, null);
+
+    transientCacheSize = cfg.getInt(ConfigSolr.CfgProp.SOLR_TRANSIENTCACHESIZE, Integer.MAX_VALUE);
 
     if (shareSchema) {
       indexSchemaCache = new ConcurrentHashMap<String,IndexSchema>();
     }
-    adminHandler = cfg.get(ConfigSolr.ConfLevel.SOLR_CORES, "adminHandler", null);
-    managementPath = cfg.get(ConfigSolr.ConfLevel.SOLR_CORES, "managementPath", null);
-
-    transientCacheSize = cfg.getInt(ConfigSolr.ConfLevel.SOLR_CORES, "transientCacheSize", Integer.MAX_VALUE);
 
     zkClientTimeout = Integer.parseInt(System.getProperty("zkClientTimeout",
         Integer.toString(zkClientTimeout)));
@@ -1336,7 +1341,10 @@ public class CoreContainer
   }
 
   /** Persists the cores config file in a user provided file. */
+  //TODO: obsolete in SOLR 5.0
   public void persistFile(File file) {
+    if (cfg != null && cfg.is50OrLater()) return;
+
     log.info("Persisting cores config to " + (file == null ? configFile : file));
 
     
@@ -1347,27 +1355,31 @@ public class CoreContainer
     
     // <solr attrib="value"> <cores attrib="value">
     Map<String,String> coresAttribs = new HashMap<String,String>();
-    addCoresAttrib(coresAttribs, "adminPath", this.adminPath, null);
+    addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_ADMINPATH, "adminPath", this.adminPath, null);
-    addCoresAttrib(coresAttribs, "adminHandler", this.adminHandler, null);
+    addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_ADMINHANDLER, "adminHandler", this.adminHandler, null);
-    addCoresAttrib(coresAttribs, "shareSchema",
+    addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_SHARESCHEMA,"shareSchema",
         Boolean.toString(this.shareSchema),
         Boolean.toString(DEFAULT_SHARE_SCHEMA));
-    addCoresAttrib(coresAttribs, "host", this.host, null);
+    addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_HOST, "host", this.host, null);
 
     if (! (null == defaultCoreName || defaultCoreName.equals("")) ) {
       coresAttribs.put("defaultCoreName", defaultCoreName);
     }
 
-    addCoresAttrib(coresAttribs, "hostPort", this.hostPort, DEFAULT_HOST_PORT);
+    addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_HOSTPORT, "hostPort", this.hostPort, DEFAULT_HOST_PORT);
-    addCoresAttrib(coresAttribs, "zkClientTimeout",
+    addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_ZKCLIENTTIMEOUT, "zkClientTimeout",
         intToString(this.zkClientTimeout),
         Integer.toString(DEFAULT_ZK_CLIENT_TIMEOUT));
-    addCoresAttrib(coresAttribs, "hostContext", this.hostContext, DEFAULT_HOST_CONTEXT);
+    addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_HOSTCONTEXT, "hostContext",
-    addCoresAttrib(coresAttribs, "leaderVoteWait", this.leaderVoteWait, LEADER_VOTE_WAIT);
+        this.hostContext, DEFAULT_HOST_CONTEXT);
-    addCoresAttrib(coresAttribs, "coreLoadThreads", Integer.toString(this.coreLoadThreads), Integer.toString(CORE_LOAD_THREADS));
+    addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_LEADERVOTEWAIT, "leaderVoteWait",
+        this.leaderVoteWait, LEADER_VOTE_WAIT);
+    addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_CORELOADTHREADS, "coreLoadThreads",
+        Integer.toString(this.coreLoadThreads), Integer.toString(CORE_LOAD_THREADS));
     if (transientCacheSize != Integer.MAX_VALUE) { // This test
     // is a consequence of testing. I really hate it.
-      addCoresAttrib(coresAttribs, "transientCacheSize", Integer.toString(this.transientCacheSize), Integer.toString(Integer.MAX_VALUE));
+      addCoresAttrib(coresAttribs, ConfigSolr.CfgProp.SOLR_TRANSIENTCACHESIZE, "transientCacheSize",
+          Integer.toString(this.transientCacheSize), Integer.toString(Integer.MAX_VALUE));
     }
 
     coreMaps.persistCores(cfg, containerProperties, rootSolrAttribs, coresAttribs, file, configFile, loader);
@@ -1378,18 +1390,21 @@ public class CoreContainer
     return Integer.toString(integer);
   }
 
-  private void addCoresAttrib(Map<String,String> coresAttribs, String attribName, String attribValue, String defaultValue) {
+  //TODO: Obsolete in 5.0 Having to pass prop is a hack to get us to 5.0.
+  private void addCoresAttrib(Map<String,String> coresAttribs, ConfigSolr.CfgProp prop,
+                              String attribName, String attribValue, String defaultValue) {
     if (cfg == null) {
       coresAttribs.put(attribName, attribValue);
       return;
     }
     
     if (attribValue != null) {
-      String rawValue = cfg.get(ConfigSolr.ConfLevel.SOLR_CORES, attribName, null);
+      String origValue = cfg.getOrigProp(prop, null);
-      if (rawValue == null && defaultValue != null && attribValue.equals(defaultValue)) return;
 
-      if (attribValue.equals(PropertiesUtil.substituteProperty(rawValue, loader.getCoreProperties()))) {
+      if (origValue == null && defaultValue != null && attribValue.equals(defaultValue)) return;
-        coresAttribs.put(attribName, rawValue);
+
+      if (attribValue.equals(PropertiesUtil.substituteProperty(origValue, loader.getCoreProperties()))) {
+        coresAttribs.put(attribName, origValue);
       } else {
         coresAttribs.put(attribName, attribValue);
       }
@@ -1434,6 +1449,11 @@ public class CoreContainer
   String getCoreToOrigName(SolrCore core) {
     return coreMaps.getCoreToOrigName(core);
   }
+
+  public String getBadCoreMessage(String name) {
+    return cfg.getBadCoreMessage(name);
+  }
+
 }
 
 
@@ -1476,7 +1496,7 @@ class CoreMaps {
   // Trivial helper method for load, note it implements LRU on transient cores. Also note, if
   // there is no setting for max size, nothing is done and all cores go in the regular "cores" list
   protected void allocateLazyCores(final ConfigSolr cfg, final SolrResourceLoader loader) {
-    final int transientCacheSize = cfg.getInt(ConfigSolr.ConfLevel.SOLR_CORES, "transientCacheSize", Integer.MAX_VALUE);
+    final int transientCacheSize = cfg.getInt(ConfigSolr.CfgProp.SOLR_TRANSIENTCACHESIZE, Integer.MAX_VALUE);
     if (transientCacheSize != Integer.MAX_VALUE) {
       CoreContainer.log.info("Allocating transient cache for {} transient cores", transientCacheSize);
       transientCores = new LinkedHashMap<String, SolrCore>(transientCacheSize, 0.75f, true) {

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

@@ -334,4 +334,22 @@ public class CoreDescriptor {
   public void putProperty(String prop, String val) {
     coreProperties.put(prop, val);
   }
+
+  // This is particularly useful for checking if any two cores have the same
+  // data dir.
+  public String getAbsoluteDataDir() {
+    String dataDir = getDataDir();
+    if (dataDir == null) return null; // No worse than before.
+
+    if (new File(dataDir).isAbsolute()) {
+      return SolrResourceLoader.normalizeDir(
+          SolrResourceLoader.normalizeDir(dataDir));
+    }
+
+    if (coreContainer == null) return null;
+
+    return SolrResourceLoader.normalizeDir(coreContainer.getSolrHome() +
+        SolrResourceLoader.normalizeDir(dataDir));
+
+  }
 }

solr/core/src/test-files/solr/solr-50-all.xml

@@ -0,0 +1,52 @@
+<!--
+  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.
+  -->
+
+<solr>
+  <str name="adminHandler">testAdminHandler</str>
+  <int name="coreLoadThreads">11</int>
+  <str name="coreRootDirectory">${coreRootDirectory:testCoreRootDirectory}</str>
+  <str name="managementPath">testManagementPath</str>
+  <str name="sharedLib">testSharedLib</str>
+  <str name="shareSchema">${shareSchema:testShareSchema}</str>
+  <int name="transientCacheSize">66</int>
+
+  <solrcloud>
+    <int name="distribUpdateConnTimeout">22</int>
+    <int name="distribUpdateSoTimeout">33</int>
+    <int name="leaderVoteWait">55</int>
+    <str name="host">testHost</str>
+    <str name="hostContext">testHostContext</str>
+    <int name="hostPort">${hostPort:44}</int>
+    <int name="zkClientTimeout">77</int>
+    <str name="zkHost">testZkHost</str>
+  </solrcloud>
+
+  <logging>
+    <str name="class">testLoggingClass</str>
+    <str name="enabled">testLoggingEnabled</str>
+    <watcher>
+      <int name="size">88</int>
+      <int name="threshold">99</int>
+    </watcher>
+  </logging>
+
+  <shardHandlerFactory name="testShardHandlerFactory" class="testHttpShardHandlerFactory">
+    <int name="socketTimeout">${socketTimeout:100}</int>
+    <int name="connTimeout">${connTimeout:110}</int>
+  </shardHandlerFactory>
+
+</solr>

solr/core/src/test-files/solr/solr-stress-new.xml

@@ -19,21 +19,16 @@
 <!--
  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}">
+<solr>
-
+  <solrcloud>
-  <!--
+    <str name="host">127.0.0.1</str>
-  adminPath: RequestHandler path to manage cores.
+    <int name="hostPort">8983</int>
-    If 'null' (or absent), cores will not be manageable via request handler
+    <str name="hostContext">${hostContext:solr}</str>
-  -->
+  </solrcloud>
-  <cores adminPath="/admin/cores" defaultCoreName="collection1" host="127.0.0.1" hostPort="${hostPort:8983}"
-         hostContext="${hostContext:solr}" autoDiscoverCores="true" >
-    <shardHandlerFactory name="shardHandlerFactory" class="HttpShardHandlerFactory">
-      <int name="socketTimeout">${socketTimeout:120000}</int>
-      <int name="connTimeout">${connTimeout:15000}</int>
-    </shardHandlerFactory>
-  </cores>
 
+  <shardHandlerFactory name="shardHandlerFactory" class="HttpShardHandlerFactory">
+    <int name="socketTimeout">${socketTimeout:120000}</int>
+    <int name="connTimeout">${connTimeout:15000}</int>
+  </shardHandlerFactory>
 </solr>

solr/core/src/test-files/solr/solr-stress.properties

@@ -1,22 +0,0 @@
-# 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.
-persistent=true
-cores.defaultCoreName=collection1
-cores.transientCacheSize=10
-cores.adminPath=/admin/cores
-host=127.0.0.1
-hostPort=${hostPort:8983}
-hostContext=${hostContext:solr}
-

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

@@ -28,7 +28,6 @@ import org.junit.Test;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.FileOutputStream;
-import java.io.StringReader;
 import java.util.Properties;
 import java.util.Set;
 
@@ -42,17 +41,24 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
 
   private final File solrHomeDirectory = new File(TEMP_DIR, "org.apache.solr.core.TestCoreDiscovery" + File.separator + "solrHome");
 
-  private void setMeUp() throws Exception {
+  private void setMeUp(String alternateCoreDir) throws Exception {
     if (solrHomeDirectory.exists()) {
       FileUtils.deleteDirectory(solrHomeDirectory);
     }
     assertTrue("Failed to mkdirs workDir", solrHomeDirectory.mkdirs());
+
     System.setProperty("solr.solr.home", solrHomeDirectory.getAbsolutePath());
+    String xmlStr = SOLR_XML;
+    if (alternateCoreDir != null) {
+      xmlStr = xmlStr.replace("<solr>", "<solr> <str name=\"coreRootDirectory\">" + alternateCoreDir + "</str> ");
+    }
+    File tmpFile = new File(solrHomeDirectory, ConfigSolr.SOLR_XML_FILE);
+    FileUtils.write(tmpFile, xmlStr, IOUtils.CHARSET_UTF_8.toString());
+
   }
 
-  private void addSolrXml() throws Exception {
+  private void setMeUp() throws Exception {
-    File tmpFile = new File(solrHomeDirectory, ConfigSolr.SOLR_XML_FILE);
+    setMeUp(null);
-    FileUtils.write(tmpFile, SOLR_XML, IOUtils.CHARSET_UTF_8.toString());
   }
 
   private Properties makeCorePropFile(String name, boolean isLazy, boolean loadOnStartup, String... extraProps) {
@@ -63,6 +69,7 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
     props.put(CoreDescriptor.CORE_TRANSIENT, Boolean.toString(isLazy));
     props.put(CoreDescriptor.CORE_LOADONSTARTUP, Boolean.toString(loadOnStartup));
     props.put(CoreDescriptor.CORE_DATADIR, "${core.dataDir:stuffandnonsense}");
+    props.put(CoreDescriptor.CORE_INSTDIR, "totallybogus"); // For testing that this property is ignored if present.
 
     for (String extra : extraProps) {
       String[] parts = extra.split("=");
@@ -72,13 +79,8 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
     return props;
   }
 
-  private void addCoreWithProps(Properties stockProps) throws Exception {
+  private void addCoreWithProps(Properties stockProps, File propFile) throws Exception {
-
+    if (!propFile.getParentFile().exists()) propFile.getParentFile().mkdirs();
-    File propFile = new File(solrHomeDirectory,
-        stockProps.getProperty(CoreDescriptor.CORE_NAME) + File.separator + ConfigSolr.CORE_PROP_FILE);
-    File parent = propFile.getParentFile();
-    assertTrue("Failed to mkdirs for " + parent.getAbsolutePath(), parent.mkdirs());
-
     FileOutputStream out = new FileOutputStream(propFile);
     try {
       stockProps.store(out, null);
@@ -86,7 +88,17 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
       out.close();
     }
 
-    addConfFiles(new File(parent, "conf"));
+    addConfFiles(new File(propFile.getParent(), "conf"));
+
+  }
+
+  private void addCoreWithProps(Properties stockProps) throws Exception {
+
+    File propFile = new File(solrHomeDirectory,
+        stockProps.getProperty(CoreDescriptor.CORE_NAME) + File.separator + ConfigSolr.CORE_PROP_FILE);
+    File parent = propFile.getParentFile();
+    assertTrue("Failed to mkdirs for " + parent.getAbsolutePath(), parent.mkdirs());
+    addCoreWithProps(stockProps, propFile);
   }
 
   // For testing error condition of having multiple cores with the same name.
@@ -114,10 +126,6 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
     FileUtils.copyFile(new File(top, "solrconfig-minimal.xml"), new File(confDir, "solrconfig-minimal.xml"));
   }
 
-  private void addConfigsForBackCompat() throws Exception {
-    addConfFiles(new File(solrHomeDirectory, "collection1" + File.separator + "conf"));
-  }
-
   private CoreContainer init() throws Exception {
 
     CoreContainer.Initializer init = new CoreContainer.Initializer();
@@ -138,23 +146,22 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
   // Test the basic setup, create some dirs with core.properties files in them, but solr.xml has discoverCores
   // set and insure that we find all the cores and can load them.
   @Test
-  public void testDiscover() throws Exception {
+  public void testDiscovery() throws Exception {
     setMeUp();
-    addSolrXml();
+
     // name, isLazy, loadOnStartup
-    addCoreWithProps(makeCorePropFile("core1", false, true));
+    addCoreWithProps(makeCorePropFile("core1", false, true, "dataDir=core1"));
-    addCoreWithProps(makeCorePropFile("core2", false, false));
+    addCoreWithProps(makeCorePropFile("core2", false, false, "dataDir=core2"));
 
     // I suspect what we're adding in here is a "configset" rather than a schema or solrconfig.
     //
-    addCoreWithProps(makeCorePropFile("lazy1", true, false));
+    addCoreWithProps(makeCorePropFile("lazy1", true, false, "dataDir=lazy1"));
 
     CoreContainer cc = init();
     try {
-      Properties props = cc.containerProperties;
+      assertNull("adminPath no longer allowed in solr.xml", cc.getAdminPath());
+      assertNull("defaultCore no longer allowed in solr.xml", cc.getDefaultCoreName());
 
-      assertEquals("/admin/cores", cc.getAdminPath());
-      assertEquals("defcore", cc.getDefaultCoreName());
       assertEquals("222.333.444.555", cc.getHost());
       assertEquals("6000", cc.getHostPort());
       assertEquals("solrprop", cc.getHostContext());
@@ -169,10 +176,13 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
       CoreDescriptor desc = core1.getCoreDescriptor();
       assertEquals("core1", desc.getProperty("solr.core.name"));
 
+      // Prove we're ignoring this even though it's set in the properties file
+      assertFalse("InstanceDir should be ignored", desc.getProperty("solr.core.instanceDir").contains("totallybogus"));
+
       // This is too long and ugly to put in. Besides, it varies.
       assertNotNull(desc.getProperty("solr.core.instanceDir"));
 
-      assertEquals("stuffandnonsense", desc.getProperty("solr.core.dataDir"));
+      assertEquals("core1", desc.getProperty("solr.core.dataDir"));
       assertEquals("solrconfig-minimal.xml", desc.getProperty("solr.core.configName"));
       assertEquals("schema-tiny.xml", desc.getProperty("solr.core.schemaName"));
 
@@ -188,132 +198,49 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
     }
   }
 
-
-  // Check that the various flavors of persistence work, including saving the state of a core when it's being swapped
-  // out. Added a test in here to insure that files that have config variables are saved with the config vars not the
-  // substitutions.
   @Test
-  public void testPersistTrue() throws Exception {
+  public void testAlternateCoreDir() throws Exception {
-    setMeUp();
+    File alt = new File(TEMP_DIR, "alternateCoreDir");
-    addSolrXml();
+    if (alt.exists()) FileUtils.deleteDirectory(alt);
-    System.setProperty("solr.persistent", "true");
+    alt.mkdirs();
-
+    setMeUp(alt.getAbsolutePath());
-    Properties special = makeCorePropFile("core1", false, true);
+    addCoreWithProps(makeCorePropFile("core1", false, true, "dataDir=core1"),
-    special.put(CoreDescriptor.CORE_INSTDIR, "${core1inst:anothersillypath}");
+        new File(alt, "core1" + File.separator + ConfigSolr.CORE_PROP_FILE));
-    addCoreWithProps(special);
+    addCoreWithProps(makeCorePropFile("core2", false, false, "dataDir=core2"),
-    addCoreWithProps(makeCorePropFile("core2", false, false));
+        new File(alt, "core2" + File.separator + ConfigSolr.CORE_PROP_FILE));
-    addCoreWithProps(makeCorePropFile("lazy1", true, true));
-    addCoreWithProps(makeCorePropFile("lazy2", true, true));
-    addCoreWithProps(makeCorePropFile("lazy3", true, false));
-
-    System.setProperty("core1inst", "core1");
     CoreContainer cc = init();
-    SolrCore coreC1 = cc.getCore("core1");
-    addCoreProps(coreC1, "addedPropC1=addedC1", "addedPropC1B=foo", "addedPropC1C=bar");
-
-    SolrCore coreC2 = cc.getCore("core2");
-    addCoreProps(coreC2, "addedPropC2=addedC2", "addedPropC2B=foo", "addedPropC2C=bar");
-
-    SolrCore coreL1 = cc.getCore("lazy1");
-    addCoreProps(coreL1, "addedPropL1=addedL1", "addedPropL1B=foo", "addedPropL1C=bar");
-
-    SolrCore coreL2 = cc.getCore("lazy2");
-    addCoreProps(coreL2, "addedPropL2=addedL2", "addedPropL2B=foo", "addedPropL2C=bar");
-
-    SolrCore coreL3 = cc.getCore("lazy3");
-    addCoreProps(coreL3, "addedPropL3=addedL3", "addedPropL3B=foo", "addedPropL3C=bar");
-
     try {
-      cc.persist();
+      SolrCore core1 = cc.getCore("core1");
-
+      SolrCore core2 = cc.getCore("core2");
-      // Insure that one of the loaded cores was swapped out, with a cache size of 2 lazy1 should be gone.
+      assertNotNull(core1);
-      TestLazyCores.checkInCores(cc, "core1", "core2", "lazy2", "lazy3");
+      assertNotNull(core2);
-      TestLazyCores.checkNotInCores(cc, "lazy1");
+      core1.close();
-
+      core2.close();
-      Properties orig = makeCorePropFile("core1", false, true);
-      orig.put(CoreDescriptor.CORE_INSTDIR, "${core1inst:anothersillypath}");
-      checkCoreProps(orig, "addedPropC1=addedC1", "addedPropC1B=foo", "addedPropC1C=bar");
-
-      orig = makeCorePropFile("core2", false, false);
-      checkCoreProps(orig, "addedPropC2=addedC2", "addedPropC2B=foo", "addedPropC2C=bar");
-
-      // This test insures that a core that was swapped out has its properties file persisted. Currently this happens
-      // as the file is removed from the cache.
-      orig = makeCorePropFile("lazy1", true, true);
-      checkCoreProps(orig, "addedPropL1=addedL1", "addedPropL1B=foo", "addedPropL1C=bar");
-
-      orig = makeCorePropFile("lazy2", true, true);
-      checkCoreProps(orig, "addedPropL2=addedL2", "addedPropL2B=foo", "addedPropL2C=bar");
-
-      orig = makeCorePropFile("lazy3", true, false);
-      checkCoreProps(orig, "addedPropL3=addedL3", "addedPropL3B=foo", "addedPropL3C=bar");
-
-      coreC1.close();
-      coreC2.close();
-      coreL1.close();
-      coreL2.close();
-      coreL3.close();
-
-    } finally {
-      cc.shutdown();
-    }
-  }
-
-  // Make sure that, even if we do call persist, nothing's saved unless the flag is set in solr.properties.
-  @Test
-  public void testPersistFalse() throws Exception {
-    setMeUp();
-    addSolrXml();
-
-    addCoreWithProps(makeCorePropFile("core1", false, true));
-    addCoreWithProps(makeCorePropFile("core2", false, false));
-    addCoreWithProps(makeCorePropFile("lazy1", true, true));
-    addCoreWithProps(makeCorePropFile("lazy2", false, true));
-
-    CoreContainer cc = init();
-    SolrCore coreC1 = cc.getCore("core1");
-    addCoreProps(coreC1, "addedPropC1=addedC1", "addedPropC1B=foo", "addedPropC1C=bar");
-
-    SolrCore coreC2 = cc.getCore("core2");
-    addCoreProps(coreC2, "addedPropC2=addedC2", "addedPropC2B=foo", "addedPropC2C=bar");
-
-    SolrCore coreL1 = cc.getCore("lazy1");
-    addCoreProps(coreL1, "addedPropL1=addedL1", "addedPropL1B=foo", "addedPropL1C=bar");
-
-    SolrCore coreL2 = cc.getCore("lazy2");
-    addCoreProps(coreL2, "addedPropL2=addedL2", "addedPropL2B=foo", "addedPropL2C=bar");
-
-
-    try {
-      cc.persist();
-
-      checkCoreProps(makeCorePropFile("core1", false, true));
-      checkCoreProps(makeCorePropFile("core2", false, false));
-      checkCoreProps(makeCorePropFile("lazy1", true, true));
-      checkCoreProps(makeCorePropFile("lazy2", false, true));
-
-      coreC1.close();
-      coreC2.close();
-      coreL1.close();
-      coreL2.close();
     } finally {
       cc.shutdown();
+      if (alt.exists()) FileUtils.deleteDirectory(alt);
     }
   }
 
   @Test
   public void testCoresWithSameNameError() throws Exception {
     setMeUp();
-    addSolrXml();
     addCoreWithPropsDir("core1_1", makeCorePropFile("core1", false, true));
     addCoreWithPropsDir("core1_2", makeCorePropFile("core1", false, true));
-    // Should just blow up here.
     CoreContainer cc = null;
     try {
       cc = init();
-    } catch (SolrException se) {
+      String msg = cc.getBadCoreMessage("core1");
-      assertEquals("Should be returning proper error code of 500", 500, se.code());
+      assertTrue("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
-      assertTrue(se.getCause().getMessage().indexOf("More than one core defined for core named core1") != -1);
+      assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      try {
+        cc.getCore("core1");
+      } catch (SolrException se) {
+        assertEquals("Should be returning proper error code of 500", 500, se.code());
+        msg = se.getMessage();
+        assertTrue("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
+        assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      }
     } finally {
       if (cc != null) {
         cc.shutdown();
@@ -322,18 +249,33 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
   }
 
   @Test
-  public void testCoresWithSameDataDirError() throws Exception{
+  public void testCoresWithSameDataDirError() throws Exception {
     setMeUp();
-    addSolrXml();
     addCoreWithProps(makeCorePropFile("core1", false, true, "dataDir=" + solrHomeDirectory + "datadir"));
     addCoreWithProps(makeCorePropFile("core2", false, true, "dataDir=" + solrHomeDirectory + "datadir"));
-    // Should just blow up here.
     CoreContainer cc = null;
     try {
       cc = init();
-    } catch (SolrException se) {
+      String msg = cc.getBadCoreMessage("core2");
-      assertEquals("Should be returning proper error code of 500", 500, se.code());
+      assertFalse("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
-      assertTrue(se.getCause().getMessage().indexOf("More than one core points to data dir") != -1);
+      assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      try {
+        cc.getCore("core1");
+      } catch (SolrException se) {
+        assertEquals("Should be returning proper error code of 500", 500, se.code());
+        msg = se.getMessage();
+        assertFalse("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
+        assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      }
+      try {
+        cc.getCore("core2");
+      } catch (SolrException se) {
+        assertEquals("Should be returning proper error code of 500", 500, se.code());
+        msg = se.getMessage();
+        assertFalse("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
+        assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      }
+
     } finally {
       if (cc != null) {
         cc.shutdown();
@@ -344,16 +286,22 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
   @Test
   public void testCoresWithSameNameErrorTransient() throws Exception {
     setMeUp();
-    addSolrXml();
     addCoreWithPropsDir("core1_1", makeCorePropFile("core1", true, false));
     addCoreWithPropsDir("core1_2", makeCorePropFile("core1", true, false));
-    // Should just blow up here.
     CoreContainer cc = null;
     try {
       cc = init();
-    } catch (SolrException se) {
+      String msg = cc.getBadCoreMessage("core1");
-      assertEquals("Should be returning proper error code of 500", 500, se.code());
+      assertTrue("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
-      assertTrue(se.getCause().getMessage().indexOf("More than one core defined for core named core1") != -1);
+      assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      try {
+        cc.getCore("core1");
+      } catch (SolrException se) {
+        assertEquals("Should be returning proper error code of 500", 500, se.code());
+        msg = se.getMessage();
+        assertTrue("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
+        assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      }
     } finally {
       if (cc != null) {
         cc.shutdown();
@@ -362,18 +310,25 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
   }
 
   @Test
-  public void testCoresWithSameDataDirErrorTransient() throws Exception{
+  public void testCoresWithSameDataDirErrorTransient() throws Exception {
     setMeUp();
-    addSolrXml();
     addCoreWithProps(makeCorePropFile("core1", true, false, "dataDir=" + solrHomeDirectory + "datadir"));
     addCoreWithProps(makeCorePropFile("core2", true, false, "dataDir=" + solrHomeDirectory + "datadir"));
     // Should just blow up here.
     CoreContainer cc = null;
     try {
       cc = init();
-    } catch (SolrException se) {
+      String msg = cc.getBadCoreMessage("core1");
-      assertEquals("Should be returning proper error code of 500", 500, se.code());
+      assertFalse("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
-      assertTrue(se.getCause().getMessage().indexOf("More than one core points to data dir") != -1);
+      assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      try {
+        cc.getCore("core1");
+      } catch (SolrException se) {
+        assertEquals("Should be returning proper error code of 500", 500, se.code());
+        msg = se.getMessage();
+        assertFalse("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
+        assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      }
     } finally {
       if (cc != null) {
         cc.shutdown();
@@ -383,18 +338,25 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
 
 
   @Test
-  public void testCoresWithSameNameErrorboth() throws Exception {
+  public void testCoresWithSameNameErrorBoth() throws Exception {
     setMeUp();
-    addSolrXml();
+    addCoreWithPropsDir("core1_1", makeCorePropFile("core1", true, false));
-    addCoreWithPropsDir("core1_1", makeCorePropFile("core1", true,  false));
     addCoreWithPropsDir("core1_2", makeCorePropFile("core1", false, false));
     // Should just blow up here.
     CoreContainer cc = null;
     try {
       cc = init();
-    } catch (SolrException se) {
+      String msg = cc.getBadCoreMessage("core1");
-      assertEquals("Should be returning proper error code of 500", 500, se.code());
+      assertTrue("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
-      assertTrue(se.getCause().getMessage().indexOf("More than one core defined for core named core1") != -1);
+      assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      try {
+        cc.getCore("core1");
+      } catch (SolrException se) {
+        assertEquals("Should be returning proper error code of 500", 500, se.code());
+        msg = se.getMessage();
+        assertTrue("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
+        assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      }
     } finally {
       if (cc != null) {
         cc.shutdown();
@@ -402,19 +364,41 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
     }
   }
 
+
   @Test
-  public void testCoresWithSameDataDirErrorBoth() throws Exception{
+  public void testCoresWithSameDataDirErrorBoth() throws Exception {
     setMeUp();
-    addSolrXml();
+    addCoreWithProps(makeCorePropFile("core1", false, false, "dataDir=" + solrHomeDirectory + "/datadir"));
-    addCoreWithProps(makeCorePropFile("core1", false, false, "dataDir=" + solrHomeDirectory + "datadir"));
+    addCoreWithProps(makeCorePropFile("core2", true, false, "dataDir=" + solrHomeDirectory + "/datadir"));
-    addCoreWithProps(makeCorePropFile("core2", true,  false, "dataDir=" + solrHomeDirectory + "datadir"));
     // Should just blow up here.
     CoreContainer cc = null;
     try {
       cc = init();
-    } catch (SolrException se) {
+      String msg = cc.getBadCoreMessage("core2");
-      assertEquals("Should be returning proper error code of 500", 500, se.code());
+      assertFalse("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
-      assertTrue(se.getCause().getMessage().indexOf("More than one core points to data dir") != -1);
+      assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+
+      msg = cc.getBadCoreMessage("core1");
+      assertFalse("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
+      assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+
+      try {
+        cc.getCore("core1");
+      } catch (SolrException se) {
+        assertEquals("Should be returning proper error code of 500", 500, se.code());
+        msg = se.getMessage();
+        assertFalse("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
+        assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      }
+      try {
+        cc.getCore("core2");
+      } catch (SolrException se) {
+        assertEquals("Should be returning proper error code of 500", 500, se.code());
+        msg = se.getMessage();
+        assertFalse("Should have found multiple cores with same name", msg.contains("More than one core defined for core named 'core1'"));
+        assertTrue("Should have found multiple cores with same data dir", msg.contains("More than one core points to data dir"));
+      }
+
     } finally {
       if (cc != null) {
         cc.shutdown();
@@ -422,41 +406,14 @@ public class TestCoreDiscovery extends SolrTestCaseJ4 {
     }
   }
 
-  void addCoreProps(SolrCore core, String... propPairs) {
-    for (String keyval : propPairs) {
-      String[] pair = keyval.split("=");
-      core.getCoreDescriptor().putProperty(pair[0], pair[1]);
-    }
-  }
-
-  // Insure that the properties in the core passed in are exactly what's in the default core.properties below plus
-  // whatever extra is passed in.
-  void checkCoreProps(Properties orig, String... extraProps) throws Exception {
-    // Read the persisted file.
-    Properties props = new Properties();
-    File propParent = new File(solrHomeDirectory, orig.getProperty(CoreDescriptor.CORE_NAME));
-    FileInputStream in = new FileInputStream(new File(propParent, ConfigSolr.CORE_PROP_FILE));
-    try {
-      props.load(in);
-    } finally {
-      in.close();
-    }
-    Set<String> propSet = props.stringPropertyNames();
-
-    assertEquals("Persisted properties should NOT contain extra properties", propSet.size(), orig.size());
-
-    for (String prop : orig.stringPropertyNames()) {
-      assertEquals("Original and new properties should be equal for " + prop, props.getProperty(prop), orig.getProperty(prop));
-    }
-    for (String prop : extraProps) {
-      String[] pair = prop.split("=");
-      assertNull("Modified parameters should not be present for " + prop, props.getProperty(pair[0]));
-    }
-  }
-
   // For testing whether finding a solr.xml overrides looking at solr.properties
-  private final static String SOLR_XML = " <solr persistent=\"${persistent:false}\"> " +
+  private final static String SOLR_XML = "<solr> " +
-      "<cores autoDiscoverCores=\"true\" adminPath=\"/admin/cores\" defaultCoreName=\"defcore\" transientCacheSize=\"2\" " +
+      "<int name=\"transientCacheSize\">2</int> " +
-       " hostContext=\"solrprop\" zkClientTimeout=\"20\" host=\"222.333.444.555\" hostPort=\"6000\" />  " +
+      "<solrcloud> " +
+      "<str name=\"hostContext\">solrprop</str> " +
+      "<int name=\"zkClientTimeout\">20</int> " +
+      "<str name=\"host\">222.333.444.555</str> " +
+      "<int name=\"hostPort\">6000</int>  " +
+      "</solrcloud> " +
       "</solr>";
 }

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

@@ -0,0 +1,120 @@
+package org.apache.solr.core;
+
+/*
+ * 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.
+ */
+
+import org.apache.commons.io.FileUtils;
+import org.apache.solr.SolrTestCaseJ4;
+import org.junit.Test;
+import org.xml.sax.SAXException;
+
+import javax.xml.parsers.ParserConfigurationException;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+
+public class TestSolrXml extends SolrTestCaseJ4 {
+  private final File solrHome = new File(TEMP_DIR, TestSolrXml.getClassName() + File.separator + "solrHome");
+
+  @Test
+  public void testAllInfoPresent() throws IOException, ParserConfigurationException, SAXException {
+    CoreContainer cc = null;
+    File testSrcRoot = new File(SolrTestCaseJ4.TEST_HOME());
+    FileUtils.copyFile(new File(testSrcRoot, "solr-50-all.xml"), new File(solrHome, "solr.xml"));
+    try {
+      InputStream is = new FileInputStream(new File(solrHome, "solr.xml"));
+      ConfigSolr cfg = new ConfigSolrXml(new SolrResourceLoader("solr/collection1"), null, is, null, false, null);
+
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_ADMINHANDLER, null), "testAdminHandler");
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_CORELOADTHREADS, 0), 11);
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_COREROOTDIRECTORY, null), "testCoreRootDirectory");
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_DISTRIBUPDATECONNTIMEOUT, 0), 22);
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_DISTRIBUPDATESOTIMEOUT, 0), 33);
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_HOST, null), "testHost");
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_HOSTCONTEXT, null), "testHostContext");
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_HOSTPORT, 0), 44);
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_LEADERVOTEWAIT, 0), 55);
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_LOGGING_CLASS, null), "testLoggingClass");
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_LOGGING_ENABLED, null), "testLoggingEnabled");
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_LOGGING_WATCHER_SIZE, 0), 88);
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_LOGGING_WATCHER_THRESHOLD, 0), 99);
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_MANAGEMENTPATH, null), "testManagementPath");
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_SHAREDLIB, null), "testSharedLib");
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_SHARDHANDLERFACTORY_CLASS, null), "testHttpShardHandlerFactory");
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_SHARDHANDLERFACTORY_CONNTIMEOUT, 0), 110);
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_SHARDHANDLERFACTORY_NAME, null), "testShardHandlerFactory");
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT, 0), 100);
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_SHARESCHEMA, null), "testShareSchema");
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_TRANSIENTCACHESIZE, 0), 66);
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_ZKCLIENTTIMEOUT, 0), 77);
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_ZKHOST, null), "testZkHost");
+      assertNull("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_PERSISTENT, null));
+      assertNull("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_CORES_DEFAULT_CORE_NAME, null));
+      assertNull("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_ADMINPATH, null));
+    } finally {
+      if (cc != null) cc.shutdown();
+    }
+  }
+
+  // Test  a few property substitutions that happen to be in solr-50-all.xml.
+  public void testPropretySub() throws IOException, ParserConfigurationException, SAXException {
+
+    String coreRoot = System.getProperty("coreRootDirectory");
+    String hostPort = System.getProperty("hostPort");
+    String shareSchema = System.getProperty("shareSchema");
+    String socketTimeout = System.getProperty("socketTimeout");
+    String connTimeout = System.getProperty("connTimeout");
+    System.setProperty("coreRootDirectory", "myCoreRoot");
+    System.setProperty("hostPort", "8888");
+    System.setProperty("shareSchema", "newShareSchema");
+    System.setProperty("socketTimeout", "220");
+    System.setProperty("connTimeout", "200");
+
+    CoreContainer cc = null;
+    File testSrcRoot = new File(SolrTestCaseJ4.TEST_HOME());
+    FileUtils.copyFile(new File(testSrcRoot, "solr-50-all.xml"), new File(solrHome, "solr.xml"));
+    try {
+      InputStream is = new FileInputStream(new File(solrHome, "solr.xml"));
+      ConfigSolr cfg = new ConfigSolrXml(new SolrResourceLoader("solr/collection1"), null, is, null, false, null);
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_COREROOTDIRECTORY, null), "myCoreRoot");
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_HOSTPORT, 0), 8888);
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_SHARDHANDLERFACTORY_CONNTIMEOUT, 0), 200);
+      assertEquals("Did not find expected value", cfg.getInt(ConfigSolr.CfgProp.SOLR_SHARDHANDLERFACTORY_SOCKETTIMEOUT, 0), 220);
+      assertEquals("Did not find expected value", cfg.get(ConfigSolr.CfgProp.SOLR_SHARESCHEMA, null), "newShareSchema");
+
+    } finally {
+      if (cc != null) cc.shutdown();
+      if (coreRoot != null) System.setProperty("coreRootDirectory", coreRoot);
+      else System.clearProperty("coreRootDirectory");
+
+      if (hostPort != null) System.setProperty("hostPort", hostPort);
+      else System.clearProperty("hostPort");
+
+      if (shareSchema != null) System.setProperty("shareSchema", shareSchema);
+      else System.clearProperty("shareSchema");
+
+      if (socketTimeout != null) System.setProperty("socketTimeout", socketTimeout);
+      else System.clearProperty("socketTimeout");
+
+      if (connTimeout != null) System.setProperty("connTimeout", connTimeout);
+      else System.clearProperty("connTimeout");
+
+    }
+  }
+}
+