[GitHub] [solr] dsmiley commented on a change in pull request #557: SOLR-15914 Make it super simple to add a contrib module to shared classpath

[GitBox <gi...@apache.org>]

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: issues-unsubscribe@solr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org