dsmiley commented on a change in pull request #23:
URL: https://github.com/apache/solr/pull/23#discussion_r604847464
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -748,7 +748,12 @@ public void load() {
warnUsersOfInsecureSettings();
this.backupRepoFactory = new
BackupRepositoryFactory(cfg.getBackupRepositoryPlugins());
- coreConfigService = ConfigSetService.createConfigSetService(this);
+ try {
Review comment:
If you look around at the existing code up & down, you can see that
plugins are initialized with a one-liner call without try-catch. So assuming
we want to clarify errors coming from createConfigSetService as being related
to configSet creation / initialization (Maybe), then it would be better for
that logic to go into createConfigSetService. I know I said yesterday to just
propagate the IOException but it wasn't evident at that time there was a need
to clarify to the caller that createConfigSetService didn't succeed. Based on
where IOException is thrown, it's not even an issue with the creation of
configSetService, it's the bootstrapping of a default config.
##########
File path:
solr/core/src/java/org/apache/solr/core/FileSystemConfigSetService.java
##########
@@ -30,123 +30,117 @@
import org.slf4j.LoggerFactory;
/**
- * The Solr standalone version of File System ConfigSetService.
+ * Solr Standalone File System ConfigSetService impl.
+ *
* <p>
- * Loads a ConfigSet defined by the core's configSet property,
- * looking for a directory named for the configSet property value underneath
- * a base directory. If no configSet property is set, loads the ConfigSet
- * instead from the core's instance directory.
+ * Loads a ConfigSet defined by the core's configSet property, looking for a
directory named for
+ * the configSet property value underneath a base directory. If no configSet
property is set, loads
+ * the ConfigSet instead from the core's instance directory.
*/
public class FileSystemConfigSetService extends ConfigSetService {
- private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private final Path configSetBase;
-
- public FileSystemConfigSetService(CoreContainer cc) {
- super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
- this.configSetBase = cc.getConfig().getConfigSetBaseDirectory();
- }
-
- @Override
- public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
- Path instanceDir = locateInstanceDir(cd);
- SolrResourceLoader solrResourceLoader = new
SolrResourceLoader(instanceDir, parentLoader.getClassLoader());
- return solrResourceLoader;
- }
-
- @Override
- public String configSetName(CoreDescriptor cd) {
- return (cd.getConfigSet() == null ? "instancedir " : "configset ") +
locateInstanceDir(cd);
- }
-
- @Override
- public boolean checkConfigExists(String configName) throws IOException {
- List<String> configs = listConfigs();
- for (String config : configs) {
- if (config.endsWith(configName)) {
- return true;
- }
- }
- return false;
- }
-
- @Override
- public void deleteConfig(String configName) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void deleteFilesFromConfig(String configName, List<String>
filesToDelete) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- public void copyConfig(String fromConfig, String toConfig) throws
IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void uploadConfig(String configName, Path dir) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void uploadFileToConfig(String configName, String fileName, byte[]
data, boolean overwriteOnExists) throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void setConfigMetadata(String configName, Map<String, Object> data)
throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public Map<String, Object> getConfigMetadata(String configName) throws
IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void downloadConfig(String configName, Path dir) throws IOException
{
- throw new UnsupportedOperationException();
- }
-
- @Override
- public List<String> listConfigs() throws IOException {
- return Files.list(configSetBase)
- .map(Path::toString)
- .collect(Collectors.toList());
- }
-
- @Override
- public byte[] downloadFileFromConfig(String configName, String filePath)
throws IOException {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public List<String> getAllConfigFiles(String configName) throws
IOException {
- throw new UnsupportedOperationException();
- }
-
- protected Path locateInstanceDir(CoreDescriptor cd) {
- String configSet = cd.getConfigSet();
- if (configSet == null)
- return cd.getInstanceDir();
- Path configSetDirectory = configSetBase.resolve(configSet);
- if (!Files.isDirectory(configSetDirectory))
- throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
- "Could not load configuration from directory " +
configSetDirectory);
- return configSetDirectory;
- }
-
- @Override
- protected Long getCurrentSchemaModificationVersion(String configSet,
SolrConfig solrConfig, String schemaFileName) {
- Path schemaFile =
solrConfig.getResourceLoader().getConfigPath().resolve(schemaFileName);
- try {
- return Files.getLastModifiedTime(schemaFile).toMillis();
- } catch (FileNotFoundException e) {
- return null; // acceptable
- } catch (IOException e) {
- log.warn("Unexpected exception when getting modification time of
{}", schemaFile, e);
- return null; // debatable; we'll see an error soon if there's a
real problem
- }
- }
-
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private final Path configSetBase;
+
+ public FileSystemConfigSetService(CoreContainer cc) {
+ super(cc.getResourceLoader(), cc.getConfig().hasSchemaCache());
+ this.configSetBase = cc.getConfig().getConfigSetBaseDirectory();
+ }
+
+ @Override
+ public SolrResourceLoader createCoreResourceLoader(CoreDescriptor cd) {
+ Path instanceDir = locateInstanceDir(cd);
+ SolrResourceLoader solrResourceLoader = new
SolrResourceLoader(instanceDir, parentLoader.getClassLoader());
+ return solrResourceLoader;
+ }
+
+ @Override
+ public String configSetName(CoreDescriptor cd) {
+ return (cd.getConfigSet() == null ? "instancedir " : "configset ") +
locateInstanceDir(cd);
+ }
+
+ @Override
+ public boolean checkConfigExists(String configName) throws IOException {
+ Path configSetDirectory = configSetBase.resolve(configName);
+ return Files.isDirectory(configSetDirectory);
+ }
+
+ @Override
+ public void deleteConfig(String configName) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void deleteFilesFromConfig(String configName, List<String>
filesToDelete) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ public void copyConfig(String fromConfig, String toConfig) throws
IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void uploadConfig(String configName, Path dir) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void uploadFileToConfig(String configName, String fileName, byte[]
data, boolean overwriteOnExists) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void setConfigMetadata(String configName, Map<String, Object> data)
throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public Map<String, Object> getConfigMetadata(String configName) throws
IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void downloadConfig(String configName, Path dir) throws IOException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public List<String> listConfigs() throws IOException {
+ return Files.list(configSetBase)
Review comment:
We have to use try-with-resources here. See
https://github.com/apache/solr/blob/dd22ed1a18a6e3d5b6f599c4c665b6374dc9e24d/solr/core/src/java/org/apache/solr/handler/CatStream.java#L231
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
