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/22 11:25:05 UTC
[GitHub] [solr] bruno-roustant commented on a change in pull request #37: SOLR-15274: QueryElevationComponent, no data dir
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