SOLR-1846: Eliminate support for the abortOnConfigurationError ... this also fixes SOLR-1824

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

solr/CHANGES.txt

@@ -62,6 +62,11 @@ Upgrading from Solr 1.4
 * SOLR-1876: All Analyzers and TokenStreams are now final to enforce
   the decorator pattern.  (rmuir, uschindler)
 
+* Setting abortOnConfigurationError=false is no longer supported
+  (since it has never worked properly).  Solr will now warn you if
+  you attempt to set this configuration option at all. (see SOLR-1846)
+
+
 Detailed Change List
 ----------------------
 
@@ -296,6 +301,9 @@ Bug Fixes
   or overwriteDupes=true with a signatureField that is not indexed.
   (hossman)
 
+* SOLR-1824: IndexSchema will now fail to initialize if there is a
+  problem initializing one of the fields or field types. (hossman)
+
 Other Changes
 ----------------------
 
@@ -369,6 +377,9 @@ Other Changes
   beginning of input files, these are often created by editors such as Windows
   Notepad.  (rmuir, hossman)
 
+* SOLR-1846: Eliminate support for the abortOnConfigurationError
+  option.  It has never worked very well, and in recent versions of
+  Solr hasn't worked at all.  (hossman)
   
 Build
 ----------------------

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

@@ -71,12 +71,6 @@ public class CoreContainer
   protected String solrConfigFilenameOverride;
   private String defaultCoreName = "";
   
-  // assigned by Initializer
-  private boolean defaultAbortOnConfigError = false;
-  // number of cores that either explicitly, or because of
-  // default, said to abort on config error
-  private int numCoresAbortOnConfigError = 0;
-  
   public CoreContainer() {
     solrHome = SolrResourceLoader.locateSolrHome();
   }
@@ -88,20 +82,23 @@ public class CoreContainer
   // Helper class to initialize the CoreContainer
   public static class Initializer {
     protected String solrConfigFilename = null;
-    // default to true for legacy behavior
-    protected boolean abortOnConfigurationError = true;
 
+    /**
+     * @deprecated all cores now abort on configuration error regardless of configuration
+     */
     public boolean isAbortOnConfigurationError() {
-      return abortOnConfigurationError;
+      return true;
     }
     
-    /** Note for no good reason what so ever, this method has only ever
-     * influenced the default behavior of "single core" mode.  when using
-     * solr.xml values specified this way are ignored, and false is the default.
-     * initialize() will modify this value.
+    /**
+     * @exception generates an error if you attempt to set this value to false
+     * @deprecated all cores now abort on configuration error regardless of configuration
      */
     public void setAbortOnConfigurationError(boolean abortOnConfigurationError) {
-      this.abortOnConfigurationError = abortOnConfigurationError;
+      if (false == abortOnConfigurationError)
+        throw new SolrException
+          (SolrException.ErrorCode.SERVER_ERROR,
+           "Setting abortOnConfigurationError==false is no longer supported");
     }
 
     public String getSolrConfigFilename() {
@@ -123,15 +120,11 @@ public class CoreContainer
       cores = new CoreContainer();
       cores.solrConfigFilenameOverride = solrConfigFilename;
       if (fconf.exists()) {
-        // default abortOnConfigurationError ignored in multicore
-        cores.defaultAbortOnConfigError = false;
         cores.load(solrHome, fconf);
       } else {
-        cores.defaultAbortOnConfigError = abortOnConfigurationError;
         cores.load(solrHome, new ByteArrayInputStream(DEF_SOLR_XML.getBytes()));
         cores.configFile = fconf;
       }
-      setAbortOnConfigurationError(0 < cores.numCoresAbortOnConfigError);
       
       solrConfigFilename = cores.getConfigFile().getName();
       
@@ -413,10 +406,6 @@ public class CoreContainer
     SolrResourceLoader solrLoader = new SolrResourceLoader(instanceDir, libLoader, getCoreProps(instanceDir, dcore.getPropertiesName(),dcore.getCoreProperties()));
     SolrConfig config = new SolrConfig(solrLoader, dcore.getConfigName(), null);
 
-    if (config.getBool("abortOnConfigurationError",defaultAbortOnConfigError)) {
-      numCoresAbortOnConfigError++;
-    }
-    
     IndexSchema schema = null;
     if(indexSchemaCache != null){
       //schema sharing is enabled. so check if it already is loaded

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

@@ -18,6 +18,7 @@
 package org.apache.solr.core;
 
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.util.NamedList;
 import org.apache.solr.common.util.SimpleOrderedMap;
 import org.apache.solr.request.SolrQueryRequest;
@@ -167,9 +168,12 @@ final class RequestHandlers {
             log.warn("Multiple default requestHandler registered" + " ignoring: " + old.getClass().getName()); 
         }
         log.info("created "+info.name+": " + info.className);
-      } catch (Exception e) {
-          SolrConfig.severeErrors.add( e );
+      } catch (Exception ex) {
+          SolrConfig.severeErrors.add( ex );
+          SolrException e = new SolrException
+            (ErrorCode.SERVER_ERROR, "RequestHandler init failure", ex);
           SolrException.logOnce(log,null,e);
+          throw e;
       }
     }
     for (Map.Entry<PluginInfo,SolrRequestHandler> entry : handlers.entrySet()) {

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

@@ -79,7 +79,10 @@ public class SolrConfig extends Config {
 
   /**
    * Singleton keeping track of configuration errors
+   *
+   * @deprecated All exceptions encountered during config parsing are now thrown by the respective constructors, preventing initialization.
    */
+  @Deprecated
   public static final Collection<Throwable> severeErrors = new HashSet<Throwable>();
 
   /** Creates a default instance from the solrconfig.xml. */

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

@@ -23,6 +23,7 @@ import org.apache.lucene.index.IndexWriter;
 import org.apache.lucene.search.BooleanQuery;
 import org.apache.lucene.store.Directory;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.CommonParams.EchoParamStyle;
 import org.apache.solr.common.params.SolrParams;
@@ -1583,8 +1584,17 @@ public final class SolrCore implements SolrInfoMBean {
           "solrconfig.xml uses deprecated <bool name='facet.sort'>. Please "+
           "update your config to use <string name='facet.sort'>.");
     }
+
+    if (!solrConfig.getBool("abortOnConfigurationError",true))
+      throw new SolrException(ErrorCode.SERVER_ERROR,
+                              "Setting abortOnConfigurationError==false is no longer supported");
+    if (null != solrConfig.getVal("abortOnConfigurationError", false))
+      log.warn("The abortOnConfigurationError option is no longer supported "+
+               "in solrconfig.xml.  Setting it has no effect.");
+    
   }
   
+
   public CoreDescriptor getCoreDescriptor() {
     return coreDescriptor;
   }

solr/src/java/org/apache/solr/schema/IndexSchema.java

@@ -512,11 +512,11 @@ public final class IndexSchema {
           SchemaField old = fields.put(f.getName(),f);
           if( old != null ) {
             String msg = "[schema.xml] Duplicate field definition for '"
-              + f.getName() + "' ignoring: "+old.toString();
-            
-            Throwable t = new SolrException( SolrException.ErrorCode.SERVER_ERROR, msg );
+              + f.getName() + "' [[["+old.toString()+"]]] and [[["+f.toString()+"]]]";
+            SolrException t = new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg );
             SolrException.logOnce(log,null,t);
             SolrConfig.severeErrors.add( t );
+            throw t;
           }
           log.debug("field defined: " + f);
           if( f.getDefaultValue() != null ) {
@@ -687,11 +687,12 @@ public final class IndexSchema {
       addDynamicFieldNoDupCheck(dFields, f);
     } else {
       String msg = "[schema.xml] Duplicate DynamicField definition for '"
-              + f.getName() + "' ignoring: " + f.toString();
+              + f.getName() + "'";
 
-      Throwable t = new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg);
+      SolrException t = new SolrException(SolrException.ErrorCode.SERVER_ERROR, msg);
       SolrException.logOnce(log, null, t);
       SolrConfig.severeErrors.add(t);
+      throw t;
     }
   }
 

solr/src/java/org/apache/solr/util/plugin/AbstractPluginLoader.java

@@ -24,6 +24,7 @@ import org.slf4j.LoggerFactory;
 
 import org.apache.solr.common.ResourceLoader;
 import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.util.DOMUtil;
 import org.apache.solr.core.SolrConfig;
 import org.apache.solr.core.SolrResourceLoader;
@@ -151,21 +152,25 @@ public abstract class AbstractPluginLoader<T>
           
           T old = register( name, plugin );
           if( old != null && !( name == null && !requireName ) ) {
-            throw new SolrException( SolrException.ErrorCode.SERVER_ERROR, 
+            throw new SolrException( ErrorCode.SERVER_ERROR, 
                 "Multiple "+type+" registered to the same name: "+name+" ignoring: "+old );
           }
           
           if( defaultStr != null && Boolean.parseBoolean( defaultStr ) ) {
             if( defaultPlugin != null ) {
-              throw new SolrException( SolrException.ErrorCode.SERVER_ERROR, 
+              throw new SolrException( ErrorCode.SERVER_ERROR, 
                 "Multiple default "+type+" plugins: "+defaultPlugin + " AND " + name );
             }
             defaultPlugin = plugin;
           }
         }
-        catch (Exception e) {
+        catch (Exception ex) {
+          SolrException e = new SolrException
+            (ErrorCode.SERVER_ERROR,
+             "Plugin init failure for " + type + ":" + ex.getMessage(), ex);
           SolrConfig.severeErrors.add( e );
           SolrException.logOnce(log,null,e);
+          throw e;
         }
       }
     }
@@ -176,8 +181,11 @@ public abstract class AbstractPluginLoader<T>
         init( pinfo.plugin, pinfo.node );
       }
       catch( Exception ex ) {
-        SolrConfig.severeErrors.add( ex );
-        SolrException.logOnce(log,null,ex);
+        SolrException e = new SolrException
+          (ErrorCode.SERVER_ERROR, "Plugin Initializing failure for " + type, ex);
+        SolrConfig.severeErrors.add( e );
+        SolrException.logOnce(log,null,e);
+        throw e;
       }
     }
     return defaultPlugin;
@@ -222,9 +230,12 @@ public abstract class AbstractPluginLoader<T>
                 + " ignoring: " + old);
       }
 
-    } catch (Exception e) {
-      SolrConfig.severeErrors.add(e);
-      SolrException.logOnce(log, null, e);
+    } catch (Exception ex) {
+      SolrException e = new SolrException
+        (ErrorCode.SERVER_ERROR, "Plugin init failure for " + type, ex);
+      SolrConfig.severeErrors.add( e );
+      SolrException.logOnce(log,null,e);
+      throw e;
     }
 
     // If everything needs to be registered *first*, this will initialize later
@@ -232,8 +243,11 @@ public abstract class AbstractPluginLoader<T>
       try {
         init(pinfo.plugin, pinfo.node);
       } catch (Exception ex) {
-        SolrConfig.severeErrors.add(ex);
-        SolrException.logOnce(log, null, ex);
+        SolrException e = new SolrException
+          (ErrorCode.SERVER_ERROR, "Plugin init failure for " + type, ex);
+        SolrConfig.severeErrors.add( e );
+        SolrException.logOnce(log,null,e);
+        throw e;
       }
     }
     return plugin;

solr/src/test/org/apache/solr/schema/BadIndexSchemaTest.java

@@ -17,73 +17,48 @@
 
 package org.apache.solr.schema;
 
-import java.util.LinkedList;
-import java.util.List;
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.common.SolrException.ErrorCode;
 
-import org.apache.solr.core.SolrConfig;
-import org.apache.solr.core.SolrCore;
-import org.apache.solr.util.AbstractSolrTestCase;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
-/**
- */
-public class BadIndexSchemaTest extends AbstractSolrTestCase {
+import org.junit.Before;
+import org.junit.Test;
 
-  @Override public String getSchemaFile() { return "bad-schema.xml"; }
-  @Override public String getSolrConfigFile() { return "solrconfig.xml"; }
+public class BadIndexSchemaTest extends SolrTestCaseJ4 {
 
-  @Override 
-  public void setUp() throws Exception {
-    ignoreException("_twice");
-    ignoreException("ftAgain");
-    ignoreException("fAgain");
+  private void doTest(final String schema, final String errString) 
+    throws Exception {
 
-    super.setUp();
+    ignoreException(errString);
+    try {
+      initCore( "solrconfig.xml", schema );
+    } catch (SolrException e) {
+      // short circut out if we found what we expected
+      if (-1 != e.getMessage().indexOf(errString)) return;
+
+      // otherwise, rethrow it, possibly completley unrelated
+      throw new SolrException
+        (ErrorCode.SERVER_ERROR, 
+         "Unexpected error, expected error matching: " + errString, e);
+    }
+    fail("Did not encounter any exception from: " + schema);
   }
 
-  @Override 
-  public void tearDown() throws Exception {
-    super.tearDown();
+  @Test
+  public void testSevereErrorsForDuplicateFields() throws Exception {
+    doTest("bad-schema-dup-field.xml", "fAgain");
   }
 
-  
-  private Throwable findErrorWithSubstring( List<Throwable> err, String v )
-  {
-    for( Throwable t : err ) {
-      if( t.getMessage().indexOf( v ) > 0 ) {
-        return t;
-      }
-    }
-    return null;
+  @Test
+  public void testSevereErrorsForDuplicateDynamicField() throws Exception {
+    doTest("bad-schema-dup-dynamicField.xml", "_twice");
   }
 
-  
-  public void testSevereErrorsForDuplicateNames() 
-  {
-    SolrCore core = h.getCore();
-    IndexSchema schema = core.getSchema();
-
-    for( Throwable t : SolrConfig.severeErrors ) {
-      log.info( "got ex:"+t.getMessage() );
-    }
-    
-    assertEquals( 3, SolrConfig.severeErrors.size() );
-
-    List<Throwable> err = new LinkedList<Throwable>();
-    err.addAll( SolrConfig.severeErrors );
-    
-    Throwable t = findErrorWithSubstring( err, "*_twice" );
-    assertNotNull( t );
-    err.remove( t );
-    
-    t = findErrorWithSubstring( err, "ftAgain" );
-    assertNotNull( t );
-    err.remove( t );
-    
-    t = findErrorWithSubstring( err, "fAgain" );
-    assertNotNull( t );
-    err.remove( t );
-
-    // make sure thats all of them
-    assertTrue( err.isEmpty() );
+  @Test
+  public void testSevereErrorsForDuplicateFieldType() throws Exception {
+    doTest("bad-schema-dup-fieldType.xml", "ftAgain");
   }
 }

solr/src/test/test-files/solr/conf/bad-schema-dup-dynamicField.xml

@@ -0,0 +1,43 @@
+<?xml version="1.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.
+-->
+
+<schema name="bad-schema-dup-dynamicField" version="1.0">
+  <types>
+
+    <fieldType name="string" class="solr.StrField"/>
+
+ </types>
+
+
+ <fields>
+   <field name="id" type="string" indexed="true" stored="true" multiValued="false" required="false"/>
+   <field name="signatureField" type="string" indexed="true" stored="false"/>
+    
+   <dynamicField name="*_sS" type="string"  indexed="false" stored="true"/>
+
+   <!-- BEGIN BAD STUFF -->
+   <dynamicField name="*_twice"  type="string"  indexed="true"  stored="true"/>
+   <dynamicField name="*_twice"  type="string"  indexed="true"  stored="true"/>
+   <!-- END BAD STUFF -->
+
+ </fields>
+
+ <defaultSearchField>id</defaultSearchField>
+ <uniqueKey>id</uniqueKey>
+
+</schema>

solr/src/test/test-files/solr/conf/bad-schema-dup-field.xml

@@ -16,32 +16,22 @@
  limitations under the License.
 -->
 
-<schema name="test" version="1.0">
+<schema name="bad-schema-dup-field" version="1.0">
   <types>
-
     <fieldType name="string" class="solr.StrField"/>
-    
-    <fieldType name="ftAgain" class="solr.IntField"/>
-    <fieldType name="ftAgain" class="solr.IntField"/>
-
-    <!-- this ones is ok -->
     <fieldtype name="text" class="solr.TextField" />
-
  </types>
 
 
  <fields>
-   <field name="id" type="text" indexed="true" stored="true" multiValued="false" required="false"/>
-   
-   <fieldType name="string" class="solr.StrField"/>
+   <field name="id" type="string" indexed="true" stored="true" multiValued="false" required="false"/>
    
    <field name="signatureField" type="string" indexed="true" stored="false"/>
 
+   <!-- BEGIN BAD STUFF -->
    <field name="fAgain" type="text" indexed="true" stored="true"/>
    <field name="fAgain" type="text" indexed="true" stored="true"/>
-
-   <dynamicField name="*_twice"  type="text"  indexed="true"  stored="true"/>
-   <dynamicField name="*_twice"  type="text"  indexed="true"  stored="true"/>
+   <!-- END BAD STUFF -->
 
    <dynamicField name="*_sS" type="string"  indexed="false" stored="true"/>
 

solr/src/test/test-files/solr/conf/bad-schema-dup-fieldType.xml

@@ -0,0 +1,44 @@
+<?xml version="1.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.
+-->
+
+<schema name="bad-schema-dup-fieldType" version="1.0">
+  <types>
+    
+    <fieldtype name="text" class="solr.TextField" />
+    <fieldType name="string" class="solr.StrField"/>
+    
+    <!-- BEGIN BAD STUFF -->
+    <fieldType name="ftAgain" class="solr.IntField"/>
+    <fieldType name="ftAgain" class="solr.IntField"/>
+    <!-- END BAD STUFF -->
+    
+ </types>
+
+
+ <fields>
+   <field name="id" type="string" indexed="true" stored="true" multiValued="false" required="false"/>
+   
+   <field name="signatureField" type="string" indexed="true" stored="false"/>
+   <dynamicField name="*_sS" type="string"  indexed="false" stored="true"/>
+
+ </fields>
+
+ <defaultSearchField>id</defaultSearchField>
+ <uniqueKey>id</uniqueKey>
+
+</schema>

solr/src/webapp/src/org/apache/solr/servlet/SolrDispatchFilter.java

@@ -100,10 +100,6 @@ public class SolrDispatchFilter implements Filter
       PrintWriter out = new PrintWriter( sw );
       out.println( "Severe errors in solr configuration.\n" );
       out.println( "Check your log files for more detailed information on what may be wrong.\n" );
-      out.println( "If you want solr to continue after configuration errors, change: \n");
-      out.println( " <abortOnConfigurationError>false</abortOnConfigurationError>\n" );
-      out.println( "in "+init.getSolrConfigFilename()+"\n" );
-
       for( Throwable t : SolrConfig.severeErrors ) {
         out.println( "-------------------------------------------------------------" );
         t.printStackTrace( out );