SOLR-15170: Elevation file in data dir now works in Solr Cloud. #3
Conversation
| 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)) { |
There was a problem hiding this 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.
| 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. |
There was a problem hiding this 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?
| if (zkController != null) { | ||
| cfg = new XmlConfigFile(core.getResourceLoader(), configFileName, null, null); | ||
| } else { | ||
| InputStream is = VersionedFile.getLatestFile(core.getDataDir(), configFileName); |
There was a problem hiding this 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.
There was a problem hiding this comment.
It's not removed, the block is just refactored.
|
Closing. FYI I worked on this #37 which doesn't change anything for SolrCloud but it's related nonetheless. |
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.