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/10 15:44:56 UTC
[GitHub] [solr] bruno-roustant opened a new pull request #3: SOLR-15170: Elevation file in data dir now works in Solr Cloud.
bruno-roustant opened a new pull request #3:
URL: https://github.com/apache/solr/pull/3
New test added for QueryElevation in Solr Cloud.
I verified the test failed before the fix (it detected the issue) and now passes with the fix.
----------------------------------------------------------------
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 #3: SOLR-15170: Elevation file in data dir now works in Solr Cloud.
Posted by GitBox <gi...@apache.org>.
bruno-roustant commented on a change in pull request #3:
URL: https://github.com/apache/solr/pull/3#discussion_r593414197
##########
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:
It's not removed, the block is just refactored.
----------------------------------------------------------------
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 closed pull request #3: SOLR-15170: Elevation file in data dir now works in Solr Cloud.
Posted by GitBox <gi...@apache.org>.
dsmiley closed pull request #3:
URL: https://github.com/apache/solr/pull/3
--
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 pull request #3: SOLR-15170: Elevation file in data dir now works in Solr Cloud.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #3:
URL: https://github.com/apache/solr/pull/3#issuecomment-803446021
Closing. FYI I worked on this https://github.com/apache/solr/pull/37 which doesn't change anything for SolrCloud but it's related nonetheless.
--
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 #3: SOLR-15170: Elevation file in data dir now works in Solr Cloud.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org