dsmiley commented on a change in pull request #557:
URL: https://github.com/apache/solr/pull/557#discussion_r790211840
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1581,7 +1553,7 @@ private void resetIndexDirectory(CoreDescriptor dcore,
ConfigSet coreConfig) {
df.release(dir);
df.doneWithDirectory(dir);
} catch (IOException e) {
- SolrException.log(log, e);
Review comment:
Was this accidental?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrXmlConfig.java
##########
@@ -327,6 +327,9 @@ private static NodeConfig
fillSolrSection(NodeConfig.NodeConfigBuilder builder,
case "sharedLib":
builder.setSharedLibDirectory(value);
break;
+ case "modules":
+ builder.setModules(value);
Review comment:
Shouldn't we be comma splitting at this point so that we have modules
declared as a list of strings?
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1884,8 +1855,6 @@ public void unload(String name, boolean deleteIndexDir,
boolean deleteDataDir, b
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new SolrException(ErrorCode.SERVER_ERROR, "Interrupted while
unregistering core [" + name + "] from cloud state");
- } catch (KeeperException e) {
Review comment:
and more accidental?
##########
File path: solr/core/src/java/org/apache/solr/core/NodeConfig.java
##########
@@ -360,6 +381,86 @@ public String getDefaultZkHost() {
return allowUrls;
}
+ // Configures SOLR_HOME/lib to the shared class loader
+ private void setupSharedLib() {
+ // Always add $SOLR_HOME/lib to the shared resource loader
+ Set<String> libDirs = new LinkedHashSet<>();
+ libDirs.add("lib");
+
+ // Always add $SOLR_TIP/lib to the shared resource loader, to allow
loading of i.e. /opt/solr/lib/foo.jar
+ if (getSolrInstallDir() != null) {
+
libDirs.add(getSolrInstallDir().resolve("lib").toAbsolutePath().normalize().toString());
+ }
+
+ if (!StringUtils.isBlank(getSharedLibDirectory())) {
+ List<String> sharedLibs =
Arrays.asList(getSharedLibDirectory().split("\\s*,\\s*"));
+ libDirs.addAll(sharedLibs);
+ }
+
+ addFoldersToSharedLib(libDirs);
+ }
+
+ /**
+ * Returns the modules as configured in solr.xml. Comma separated list. May
be null if not defined
+ */
+ public String getModules() {
+ return modules;
+ }
+
+ // Finds every jar in each folder and adds it to shardLib, then reloads
Lucene SPI
+ private void addFoldersToSharedLib(Set<String> libDirs) {
+ boolean modified = false;
+ // add the sharedLib to the shared resource loader before initializing cfg
based plugins
+ for (String libDir : libDirs) {
+ Path libPath = getSolrHome().resolve(libDir);
+ if (Files.exists(libPath)) {
+ try {
+ loader.addToClassLoader(SolrResourceLoader.getURLs(libPath));
+ modified = true;
+ } catch (IOException e) {
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Couldn't load libs:
" + e, e);
+ }
+ }
+ }
+ if (modified) {
+ loader.reloadLuceneSPI();
+ }
+ }
+
+ // Adds modules to shared classpath
+ private void initModules() {
+ Set<String> moduleNames =
ModuleUtils.resolveModulesFromStringOrSyspropOrEnv(getModules());
+ boolean modified = false;
+ for (String m : moduleNames) {
+ if (!ModuleUtils.moduleExists(getSolrInstallDir(), m)) {
Review comment:
This was helpful, and motivated my suggestion that getSolrInstallDir
always return non-null.
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -1711,7 +1683,6 @@ public void reload(String name, UUID coreId) {
// CoreDescriptor and we need to reload it from the disk files
CoreDescriptor cd = reloadCoreDescriptor(core.getCoreDescriptor());
solrCores.addCoreDescriptor(cd);
- Closeable oldCore = null;
Review comment:
more accidental?
##########
File path: solr/core/src/java/org/apache/solr/core/NodeConfig.java
##########
@@ -206,7 +218,16 @@ public Path getSolrDataHome() {
return solrDataHome;
}
- /**
+ /**
+ * Obtain the path of solr's binary installation directory, e.g.
<code>/opt/solr</code>
+ * @return path to install dir, or null if property 'solr.install.dir' has
not been initialized
+ */
+ public Path getSolrInstallDir() {
+ String prop =
System.getProperty(SolrDispatchFilter.SOLR_INSTALL_DIR_ATTRIBUTE);
+ return prop != null ? Paths.get(prop) : null;
Review comment:
Maybe throw an exception to ensure we never return null? It should
always be set; right? Perhaps not in tests? Ok but we could ensure this
happens.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
