dsmiley commented on a change in pull request #3:
URL: https://github.com/apache/solr/pull/3#discussion_r593378400
##########
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -256,9 +256,9 @@ protected int loadElevationConfiguration(SolrCore core)
throws Exception {
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
+ // Check if the ElevationProvider was loaded.
+ // If the config file (elevate.xml) was not found in the config dir, it
must be in the data dir.
Review comment:
This option (elevate.xml in data dir) is only something that could work
in standalone mode, not SolrCloud where it's obsoleted by the role ZooKeeper
plays. Can you make this point explicit please?
##########
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -370,14 +374,8 @@ private ElevationProvider loadElevationProvider(SolrCore
core) throws IOExceptio
}
log.info("Loading QueryElevation from data dir: {}", configFileName);
- XmlConfigFile cfg;
- ZkController zkController = core.getCoreContainer().getZkController();
- if (zkController != null) {
- cfg = new XmlConfigFile(core.getResourceLoader(), configFileName, null,
null);
- } else {
- InputStream is = VersionedFile.getLatestFile(core.getDataDir(),
configFileName);
Review comment:
Why did you remove the use of VersionedFile?
I did some digging... that mechanism was used on a different feature
originally https://issues.apache.org/jira/browse/SOLR-351 and it was argued
that for Windows OS / file system, you can't replace an already open file.
Perhaps we don't care enough about this to bother with retaining it? If we do
remove this mechanism, we need to document it in the upgrade section of the ref
guide.
##########
File path:
solr/core/src/java/org/apache/solr/handler/component/QueryElevationComponent.java
##########
@@ -226,14 +226,15 @@ protected int loadElevationConfiguration(SolrCore core)
throws Exception {
// 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
+ ElevationProvider elevationProvider = null;
+ // Check if using ZooKeeper for SolrCloud.
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);
+ if
(zkController.configFileExists(zkController.getZkStateReader().readConfigName(core.getCoreDescriptor().getCloudDescriptor().getCollectionName()),
configFileName)) {
Review comment:
I was just looking at this today coincidentally in the context of seeing
what assumptions SolrCloud makes about where configSets live outside of the
ConfigSetService abstraction. This is one of those places. I believe this
code here should instead request the config file from the SolrResourceLoader.
In SolrCloud, that will be a ZkSolrResourceLoader (instantiated by a
ConfigSetService) and should work.
----------------------------------------------------------------
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:
[email protected]
