You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/03/20 04:51:50 UTC

[GitHub] [solr] dsmiley opened a new pull request #37: SOLR-15274: QueryElevationComponent, no data dir

dsmiley opened a new pull request #37:
URL: https://github.com/apache/solr/pull/37


   https://issues.apache.org/jira/browse/SOLR-15274
   
   * Remove support for a config in a data dir. Obsolete.
   * Remove support for "versioned" (suffix) config
   * Add auto config reloading after a commit (previously was only for the data dir).  SolrCloud not supported.
   
   TODO Solr Ref Guide


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [solr] bruno-roustant commented on a change in pull request #37: SOLR-15274: QueryElevationComponent, no data dir

Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #37:
URL: https://github.com/apache/solr/pull/37#discussion_r598583826



##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -143,11 +144,25 @@
 
   protected boolean initialized;
 
+  private final Object LOCK = new Object(); // for cache*

Review comment:
       Lower case?

##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -290,70 +275,83 @@ protected void handleInitializationException(Exception exception, Initialization
 
   /**
    * Handles an exception that occurred while loading the configuration resource.
+   * The default implementation will return {@link #cacheElevationProvider} if present, while
+   * also logging the error.  If that is null (e.g. on startup) then the exception is thrown.
+   * When re-throwing, wrap in a {@link SolrException}.
    *
-   * @param e                   The exception caught.
-   * @param resourceAccessIssue <code>true</code> if the exception has been thrown
-   *                            because the resource could not be accessed (missing or cannot be read)
-   *                            or the config file is empty; <code>false</code> if the resource has
-   *                            been found and accessed but the error occurred while loading the resource
-   *                            (invalid format, incomplete or corrupted).
-   * @return The {@link ElevationProvider} to use if the exception is absorbed. If {@code null}
-   *         is returned, the {@link #NO_OP_ELEVATION_PROVIDER} is used but not cached in
-   *         the {@link ElevationProvider} cache.
-   * @throws E If the exception is not absorbed.
+   * @param e The exception caught.  It will extend {@link IOException} if there was a resource
+   *          access issue.
+   * @return The {@link ElevationProvider} to use if the exception is absorbed (vs re-thrown).
    */
-  protected <E extends Exception> ElevationProvider handleConfigLoadingException(E e, boolean resourceAccessIssue) throws E {
-    throw e;
+  protected <E extends Exception> ElevationProvider handleConfigLoadingException(E e) {
+    if (cacheElevationProvider != null) { // thus at runtime (a search is in-progress)
+      String msg = e.toString(); // declare to avoid log isEnabled check

Review comment:
       Can you explain me this comment? I don't see why we don't inline e.toString() in the log.error() parameter.

##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -218,61 +233,31 @@ private void parseUseConfiguredOrderForElevations() {
    * @return The number of elevation rules parsed.
    */
   protected int loadElevationConfiguration(SolrCore core) throws Exception {
-    synchronized (elevationProviderCache) {
-      elevationProviderCache.clear();
-      String configFileName = initArgs.get(CONFIG_FILE);
+    synchronized (LOCK) {
+      clearElevationProviderCache();
+
+      this.configFileName = initArgs.get(CONFIG_FILE);
       if (configFileName == null) {
         // Throw an exception which is handled by handleInitializationException().
         // If not overridden handleInitializationException() simply skips this exception.
         throw new InitializationException("Missing component parameter " + CONFIG_FILE + " - it has to define the path to the elevation configuration file", InitializationExceptionCause.NO_CONFIG_FILE_DEFINED);
       }
-      boolean configFileExists = false;
-      ElevationProvider elevationProvider = NO_OP_ELEVATION_PROVIDER;
-
-      // check if using ZooKeeper
-      ZkController zkController = core.getCoreContainer().getZkController();
-      if (zkController != null) {
-        // TODO : shouldn't have to keep reading the config name when it has been read before
-        configFileExists = zkController.configFileExists(zkController.getZkStateReader().readConfigName(core.getCoreDescriptor().getCloudDescriptor().getCollectionName()), configFileName);
-      } else {
-        File fC = new File(core.getResourceLoader().getConfigDir(), configFileName);
-        File fD = new File(core.getDataDir(), configFileName);
-        if (fC.exists() == fD.exists()) {
-          InitializationException e = new InitializationException("Missing config file \"" + configFileName + "\" - either " + fC.getAbsolutePath() + " or " + fD.getAbsolutePath() + " must exist, but not both", InitializationExceptionCause.MISSING_CONFIG_FILE);
-          elevationProvider = handleConfigLoadingException(e, true);
-          elevationProviderCache.put(null, elevationProvider);
-        } else if (fC.exists()) {
-          if (fC.length() == 0) {
-            InitializationException e = new InitializationException("Empty config file \"" + configFileName + "\" - " + fC.getAbsolutePath(), InitializationExceptionCause.EMPTY_CONFIG_FILE);
-            elevationProvider = handleConfigLoadingException(e, true);
-          } else {
-            configFileExists = true;
-            if (log.isInfoEnabled()) {
-              log.info("Loading QueryElevation from: {}", fC.getAbsolutePath());
-            }
-            XmlConfigFile cfg = new XmlConfigFile(core.getResourceLoader(), configFileName);
-            elevationProvider = loadElevationProvider(cfg);
-          }
-          elevationProviderCache.put(null, elevationProvider);
-        }
-      }
-      //in other words, we think this is in the data dir, not the conf dir
-      if (!configFileExists) {
-        // preload the first data
-        RefCounted<SolrIndexSearcher> searchHolder = null;
-        try {
-          searchHolder = core.getNewestSearcher(false);
-          if (searchHolder == null) {
-            elevationProvider = NO_OP_ELEVATION_PROVIDER;
-          } else {
-            IndexReader reader = searchHolder.get().getIndexReader();
-            elevationProvider = getElevationProvider(reader, core);
-          }
-        } finally {
-          if (searchHolder != null) searchHolder.decref();
+
+      // preload the first data
+      RefCounted<SolrIndexSearcher> searchHolder = null;
+      try {
+        searchHolder = core.getNewestSearcher(false);
+        if (searchHolder != null) {
+          IndexReader reader = searchHolder.get().getIndexReader();
+          getElevationProvider(reader, core); // computes and caches or throws
+          return cacheElevationProvider.size();
         }
+      } finally {
+        if (searchHolder != null) searchHolder.decref();

Review comment:
       Could we add the brackets?

##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -290,70 +275,83 @@ protected void handleInitializationException(Exception exception, Initialization
 
   /**
    * Handles an exception that occurred while loading the configuration resource.
+   * The default implementation will return {@link #cacheElevationProvider} if present, while
+   * also logging the error.  If that is null (e.g. on startup) then the exception is thrown.
+   * When re-throwing, wrap in a {@link SolrException}.
    *
-   * @param e                   The exception caught.
-   * @param resourceAccessIssue <code>true</code> if the exception has been thrown
-   *                            because the resource could not be accessed (missing or cannot be read)
-   *                            or the config file is empty; <code>false</code> if the resource has
-   *                            been found and accessed but the error occurred while loading the resource
-   *                            (invalid format, incomplete or corrupted).
-   * @return The {@link ElevationProvider} to use if the exception is absorbed. If {@code null}
-   *         is returned, the {@link #NO_OP_ELEVATION_PROVIDER} is used but not cached in
-   *         the {@link ElevationProvider} cache.
-   * @throws E If the exception is not absorbed.
+   * @param e The exception caught.  It will extend {@link IOException} if there was a resource
+   *          access issue.
+   * @return The {@link ElevationProvider} to use if the exception is absorbed (vs re-thrown).
    */
-  protected <E extends Exception> ElevationProvider handleConfigLoadingException(E e, boolean resourceAccessIssue) throws E {
-    throw e;
+  protected <E extends Exception> ElevationProvider handleConfigLoadingException(E e) {
+    if (cacheElevationProvider != null) { // thus at runtime (a search is in-progress)
+      String msg = e.toString(); // declare to avoid log isEnabled check
+      log.error(msg, e);
+      return cacheElevationProvider;
+    } else {
+      if (e instanceof SolrException || e instanceof InitializationException) {
+        throw (RuntimeException) e;
+      } else {
+        throw new SolrException(
+            SolrException.ErrorCode.SERVER_ERROR,
+            "Problem loading query elevation: " + e.toString(),
+            e);
+      }
+    }
   }
 
   /**
-   * Gets the {@link ElevationProvider} from the data dir or from the cache.
+   * Gets the {@link ElevationProvider}; typically cached.
+   * If there was a problem, it might return a previously cached or dummy entry, or possibly
+   * rethrow the exception.
    *
    * @return The cached or loaded {@link ElevationProvider}.
-   * @throws java.io.IOException                  If the configuration resource cannot be found, or if an I/O error occurs while analyzing the triggering queries.
-   * @throws org.xml.sax.SAXException                 If the configuration resource is not a valid XML content.
-   * @throws javax.xml.parsers.ParserConfigurationException If the configuration resource is not a valid XML configuration.
-   * @throws RuntimeException             If the configuration resource is not an XML content of the expected format
-   *                                      (either {@link RuntimeException} or {@link org.apache.solr.common.SolrException}).
    */
   @VisibleForTesting
-  ElevationProvider getElevationProvider(IndexReader reader, SolrCore core) throws Exception {
-    synchronized (elevationProviderCache) {
-      ElevationProvider elevationProvider;
-      elevationProvider = elevationProviderCache.get(null);
-      if (elevationProvider != null) return elevationProvider;
-
-      elevationProvider = elevationProviderCache.get(reader);
-      if (elevationProvider == null) {
-        Exception loadingException = null;
-        boolean resourceAccessIssue = false;
+  ElevationProvider getElevationProvider(IndexReader reader, SolrCore core) {

Review comment:
       Nice refactoring!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [solr] dsmiley commented on a change in pull request #37: SOLR-15274: QueryElevationComponent, no data dir

Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #37:
URL: https://github.com/apache/solr/pull/37#discussion_r598722708



##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -290,70 +275,83 @@ protected void handleInitializationException(Exception exception, Initialization
 
   /**
    * Handles an exception that occurred while loading the configuration resource.
+   * The default implementation will return {@link #cacheElevationProvider} if present, while
+   * also logging the error.  If that is null (e.g. on startup) then the exception is thrown.
+   * When re-throwing, wrap in a {@link SolrException}.
    *
-   * @param e                   The exception caught.
-   * @param resourceAccessIssue <code>true</code> if the exception has been thrown
-   *                            because the resource could not be accessed (missing or cannot be read)
-   *                            or the config file is empty; <code>false</code> if the resource has
-   *                            been found and accessed but the error occurred while loading the resource
-   *                            (invalid format, incomplete or corrupted).
-   * @return The {@link ElevationProvider} to use if the exception is absorbed. If {@code null}
-   *         is returned, the {@link #NO_OP_ELEVATION_PROVIDER} is used but not cached in
-   *         the {@link ElevationProvider} cache.
-   * @throws E If the exception is not absorbed.
+   * @param e The exception caught.  It will extend {@link IOException} if there was a resource
+   *          access issue.
+   * @return The {@link ElevationProvider} to use if the exception is absorbed (vs re-thrown).
    */
-  protected <E extends Exception> ElevationProvider handleConfigLoadingException(E e, boolean resourceAccessIssue) throws E {
-    throw e;
+  protected <E extends Exception> ElevationProvider handleConfigLoadingException(E e) {
+    if (cacheElevationProvider != null) { // thus at runtime (a search is in-progress)
+      String msg = e.toString(); // declare to avoid log isEnabled check

Review comment:
       See `gradle/validation/validate-log-calls.gradle` Erick Erickson insisted we guard many of our log calls with a log enabled check. LUCENE-7788
   IMO there shouldn't be checks on an error level but Erick had stronger opinions than I did at the time :-)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [solr] dsmiley commented on a change in pull request #37: SOLR-15274: QueryElevationComponent, no data dir

Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #37:
URL: https://github.com/apache/solr/pull/37#discussion_r598718187



##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -218,61 +233,31 @@ private void parseUseConfiguredOrderForElevations() {
    * @return The number of elevation rules parsed.
    */
   protected int loadElevationConfiguration(SolrCore core) throws Exception {
-    synchronized (elevationProviderCache) {
-      elevationProviderCache.clear();
-      String configFileName = initArgs.get(CONFIG_FILE);
+    synchronized (LOCK) {
+      clearElevationProviderCache();
+
+      this.configFileName = initArgs.get(CONFIG_FILE);
       if (configFileName == null) {
         // Throw an exception which is handled by handleInitializationException().
         // If not overridden handleInitializationException() simply skips this exception.
         throw new InitializationException("Missing component parameter " + CONFIG_FILE + " - it has to define the path to the elevation configuration file", InitializationExceptionCause.NO_CONFIG_FILE_DEFINED);
       }
-      boolean configFileExists = false;
-      ElevationProvider elevationProvider = NO_OP_ELEVATION_PROVIDER;
-
-      // check if using ZooKeeper
-      ZkController zkController = core.getCoreContainer().getZkController();
-      if (zkController != null) {
-        // TODO : shouldn't have to keep reading the config name when it has been read before
-        configFileExists = zkController.configFileExists(zkController.getZkStateReader().readConfigName(core.getCoreDescriptor().getCloudDescriptor().getCollectionName()), configFileName);
-      } else {
-        File fC = new File(core.getResourceLoader().getConfigDir(), configFileName);
-        File fD = new File(core.getDataDir(), configFileName);
-        if (fC.exists() == fD.exists()) {
-          InitializationException e = new InitializationException("Missing config file \"" + configFileName + "\" - either " + fC.getAbsolutePath() + " or " + fD.getAbsolutePath() + " must exist, but not both", InitializationExceptionCause.MISSING_CONFIG_FILE);
-          elevationProvider = handleConfigLoadingException(e, true);
-          elevationProviderCache.put(null, elevationProvider);
-        } else if (fC.exists()) {
-          if (fC.length() == 0) {
-            InitializationException e = new InitializationException("Empty config file \"" + configFileName + "\" - " + fC.getAbsolutePath(), InitializationExceptionCause.EMPTY_CONFIG_FILE);
-            elevationProvider = handleConfigLoadingException(e, true);
-          } else {
-            configFileExists = true;
-            if (log.isInfoEnabled()) {
-              log.info("Loading QueryElevation from: {}", fC.getAbsolutePath());
-            }
-            XmlConfigFile cfg = new XmlConfigFile(core.getResourceLoader(), configFileName);
-            elevationProvider = loadElevationProvider(cfg);
-          }
-          elevationProviderCache.put(null, elevationProvider);
-        }
-      }
-      //in other words, we think this is in the data dir, not the conf dir
-      if (!configFileExists) {
-        // preload the first data
-        RefCounted<SolrIndexSearcher> searchHolder = null;
-        try {
-          searchHolder = core.getNewestSearcher(false);
-          if (searchHolder == null) {
-            elevationProvider = NO_OP_ELEVATION_PROVIDER;
-          } else {
-            IndexReader reader = searchHolder.get().getIndexReader();
-            elevationProvider = getElevationProvider(reader, core);
-          }
-        } finally {
-          if (searchHolder != null) searchHolder.decref();
+
+      // preload the first data
+      RefCounted<SolrIndexSearcher> searchHolder = null;
+      try {
+        searchHolder = core.getNewestSearcher(false);
+        if (searchHolder != null) {
+          IndexReader reader = searchHolder.get().getIndexReader();
+          getElevationProvider(reader, core); // computes and caches or throws
+          return cacheElevationProvider.size();
         }
+      } finally {
+        if (searchHolder != null) searchHolder.decref();

Review comment:
       I didn't write this; merely moved around in the PR.  I think this one-liner is kind of common for searchHolder related stuff.  Even Google-Java-Format is fine with it.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [solr] dsmiley commented on a change in pull request #37: SOLR-15274: QueryElevationComponent, no data dir

Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #37:
URL: https://github.com/apache/solr/pull/37#discussion_r603766114



##########
File path: solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -143,11 +144,25 @@
 
   protected boolean initialized;
 
+  private final Object LOCK = new Object(); // for cache*

Review comment:
       It's final & immutable so all-caps seems reasonable to me




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org


[GitHub] [solr] dsmiley commented on pull request #37: SOLR-15274: QueryElevationComponent, no data dir

Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #37:
URL: https://github.com/apache/solr/pull/37#issuecomment-803407150


   The getConfigVersion method ought to move to something similar on SolrResourceLoader.  SRL is one of Solr's major APIs (albeit internal) so changing it deserves a separate issue, which I'll do.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [solr] dsmiley merged pull request #37: SOLR-15274: QueryElevationComponent, no data dir

Posted by GitBox <gi...@apache.org>.
dsmiley merged pull request #37:
URL: https://github.com/apache/solr/pull/37


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org