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:
us...@infra.apache.org
