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 2022/01/24 13:40:55 UTC
[GitHub] [solr] chatman opened a new pull request #560: SOLR-15951: Local packages
chatman opened a new pull request #560:
URL: https://github.com/apache/solr/pull/560
Ability to load packages from the filesystem, useful for first party packages.
--
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
[GitHub] [solr] noblepaul commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793268524
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -274,20 +349,25 @@ public void writeMap(EntryWriter ew) throws IOException {
version.writeMap(ew);
}
- Version(Package parent, PackageAPI.PkgVersion v) {
+ Version(Package parent, PackageAPI.PkgVersion v, Path localPkgDir) {
this.parent = parent;
this.version = v;
List<Path> paths = new ArrayList<>();
-
- List<String> errs = new ArrayList<>();
- coreContainer.getPackageStoreAPI().validateFiles(version.files, true, s -> errs.add(s));
- if(!errs.isEmpty()) {
- throw new RuntimeException("Cannot load package: " +errs);
- }
- for (String file : version.files) {
- paths.add(coreContainer.getPackageStoreAPI().getPackageStore().getRealpath(file));
+ if(localPkgDir != null) {
+ for (String file : v.files) {
+ if(file.charAt(0)== '/') file =file.substring(1);
+ paths.add( localPkgDir.resolve(file).toAbsolutePath()) ;
+ }
+ } else {
Review comment:
Yeah, I think a subclass with a different name would be better
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r791131452
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestPackages.java
##########
@@ -61,6 +61,7 @@
import org.junit.Before;
import org.junit.Test;
+import java.io.File;
Review comment:
is this needed?
##########
File path: solr/contrib/scripting/src/java/org/apache/solr/scripting/update/ScriptUpdateProcessorFactory.java
##########
@@ -49,11 +49,7 @@
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import java.security.ProtectionDomain;
-import java.util.Set;
-import java.util.LinkedHashSet;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Collection;
+import java.util.*;
Review comment:
please leave these separate.
##########
File path: solr/bin/solr
##########
@@ -2264,6 +2264,7 @@ function start_solr() {
# users who don't care about useful error msgs can override in SOLR_OPTS with +OmitStackTraceInFastThrow
"${SOLR_HOST_ARG[@]}" "-Duser.timezone=$SOLR_TIMEZONE" "-XX:-OmitStackTraceInFastThrow" \
"-XX:OnOutOfMemoryError=$SOLR_TIP/bin/oom_solr.sh $SOLR_PORT $SOLR_LOGS_DIR" \
+ "-Dsolr.packages.local.dir=$SOLR_TIP/contrib" \
Review comment:
This should be in the windows `.cmd` as well.
##########
File path: solr/packaging/build.gradle
##########
@@ -86,12 +86,20 @@ distributions {
include "NOTICE.txt"
})
+ from(project(":solr:contrib").projectDir, {
Review comment:
the `modules` directory should have a build.gradle that creates the `local_packages.json` in it's build directory, then puts that into a gradle configuration. Then this file can refer to that configuration.
same with `local_packages.json` and `manifest.json` below.
##########
File path: solr/packaging/build.gradle
##########
@@ -86,12 +86,20 @@ distributions {
include "NOTICE.txt"
})
+ from(project(":solr:contrib").projectDir, {
+ include "local_packages.json"
+ into "contrib"
+ })
+
+
from(project(":solr").projectDir, {
include "bin/**"
include "licenses/**"
exclude "licenses/README.committers.txt"
include "CHANGES.txt"
include "README.md"
+ include "local_packages.json"
Review comment:
These two files aren't under `/solr`, they are in the individual module directories...
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -52,10 +56,14 @@
public class PackageLoader implements Closeable {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String LATEST = "$LATEST";
-
+ public static final String LOCAL_PACKAGES_DIR = System.getProperty("solr.packages.local.dir");
+ public static final String LOCAL_PACKAGES_WHITELIST = System.getProperty("solr.packages.local.whitelist", "");
Review comment:
instead of `whitelist`, how about `names`, 'use' or `enable`. We are steering away from `whitelist`/`blacklist` now, and it doesn't work as well as other options.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +78,49 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(LOCAL_PACKAGES_WHITELIST, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (LOCAL_PACKAGES_DIR != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(LOCAL_PACKAGES_DIR).toPath();
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!packagesPath.toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ if (!packagesPath.resolve(LOCAL_PACKAGES_JSON).toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No file local_packages.json exists in: " + packagesPath);
+ }
+
+ try {
+ try (InputStream in = new FileInputStream(new File(packagesPath.toFile() , LOCAL_PACKAGES_JSON))) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
}
public PackageAPI getPackageAPI() {
return packageAPI;
}
public Package getPackage(String key) {
- return packageClassLoaders.get(key);
+ return packageClassLoaders.get(key);
Review comment:
wrong indentation.
##########
File path: gradle/solr/packaging.gradle
##########
@@ -60,7 +60,11 @@ configure(allprojects.findAll {project -> project.path.startsWith(":solr:contrib
// An aggregate that configures lib and test-lib in a temporary location.
task assemblePackaging(type: Sync) {
from "README.md"
-
+
+ from "manifest.json"
+
+ from "src/resources/solr-default-tika-config.xml"
Review comment:
You can put this in the `extraction` module's build.gradle using
```
assemblePackaging {
from "src/resources/solr-default-tika-config.xml"
}
```
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r791131452
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestPackages.java
##########
@@ -61,6 +61,7 @@
import org.junit.Before;
import org.junit.Test;
+import java.io.File;
Review comment:
is this needed?
##########
File path: solr/contrib/scripting/src/java/org/apache/solr/scripting/update/ScriptUpdateProcessorFactory.java
##########
@@ -49,11 +49,7 @@
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
import java.security.ProtectionDomain;
-import java.util.Set;
-import java.util.LinkedHashSet;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Collection;
+import java.util.*;
Review comment:
please leave these separate.
##########
File path: solr/bin/solr
##########
@@ -2264,6 +2264,7 @@ function start_solr() {
# users who don't care about useful error msgs can override in SOLR_OPTS with +OmitStackTraceInFastThrow
"${SOLR_HOST_ARG[@]}" "-Duser.timezone=$SOLR_TIMEZONE" "-XX:-OmitStackTraceInFastThrow" \
"-XX:OnOutOfMemoryError=$SOLR_TIP/bin/oom_solr.sh $SOLR_PORT $SOLR_LOGS_DIR" \
+ "-Dsolr.packages.local.dir=$SOLR_TIP/contrib" \
Review comment:
This should be in the windows `.cmd` as well.
##########
File path: solr/packaging/build.gradle
##########
@@ -86,12 +86,20 @@ distributions {
include "NOTICE.txt"
})
+ from(project(":solr:contrib").projectDir, {
Review comment:
the `modules` directory should have a build.gradle that creates the `local_packages.json` in it's build directory, then puts that into a gradle configuration. Then this file can refer to that configuration.
same with `local_packages.json` and `manifest.json` below.
##########
File path: solr/packaging/build.gradle
##########
@@ -86,12 +86,20 @@ distributions {
include "NOTICE.txt"
})
+ from(project(":solr:contrib").projectDir, {
+ include "local_packages.json"
+ into "contrib"
+ })
+
+
from(project(":solr").projectDir, {
include "bin/**"
include "licenses/**"
exclude "licenses/README.committers.txt"
include "CHANGES.txt"
include "README.md"
+ include "local_packages.json"
Review comment:
These two files aren't under `/solr`, they are in the individual module directories...
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -52,10 +56,14 @@
public class PackageLoader implements Closeable {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String LATEST = "$LATEST";
-
+ public static final String LOCAL_PACKAGES_DIR = System.getProperty("solr.packages.local.dir");
+ public static final String LOCAL_PACKAGES_WHITELIST = System.getProperty("solr.packages.local.whitelist", "");
Review comment:
instead of `whitelist`, how about `names`, 'use' or `enable`. We are steering away from `whitelist`/`blacklist` now, and it doesn't work as well as other options.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +78,49 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(LOCAL_PACKAGES_WHITELIST, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (LOCAL_PACKAGES_DIR != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(LOCAL_PACKAGES_DIR).toPath();
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!packagesPath.toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ if (!packagesPath.resolve(LOCAL_PACKAGES_JSON).toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No file local_packages.json exists in: " + packagesPath);
+ }
+
+ try {
+ try (InputStream in = new FileInputStream(new File(packagesPath.toFile() , LOCAL_PACKAGES_JSON))) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
}
public PackageAPI getPackageAPI() {
return packageAPI;
}
public Package getPackage(String key) {
- return packageClassLoaders.get(key);
+ return packageClassLoaders.get(key);
Review comment:
wrong indentation.
##########
File path: gradle/solr/packaging.gradle
##########
@@ -60,7 +60,11 @@ configure(allprojects.findAll {project -> project.path.startsWith(":solr:contrib
// An aggregate that configures lib and test-lib in a temporary location.
task assemblePackaging(type: Sync) {
from "README.md"
-
+
+ from "manifest.json"
+
+ from "src/resources/solr-default-tika-config.xml"
Review comment:
You can put this in the `extraction` module's build.gradle using
```
assemblePackaging {
from "src/resources/solr-default-tika-config.xml"
}
```
--
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
[GitHub] [solr] janhoy commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793352113
##########
File path: solr/bin/solr
##########
@@ -2264,6 +2264,7 @@ function start_solr() {
# users who don't care about useful error msgs can override in SOLR_OPTS with +OmitStackTraceInFastThrow
"${SOLR_HOST_ARG[@]}" "-Duser.timezone=$SOLR_TIMEZONE" "-XX:-OmitStackTraceInFastThrow" \
"-XX:OnOutOfMemoryError=$SOLR_TIP/bin/oom_solr.sh $SOLR_PORT $SOLR_LOGS_DIR" \
+ "-Dsolr.packages.local.dir=$SOLR_TIP/contrib" \
Review comment:
Should not the bundled "modules" folder always be available? Is it even necessary to make the local packages dir customizable like this? I mean, if you perhaps have your own inhouse pkg that you would want to load in a "trusted" way, you would either copy it into the "modules" folder or you'd want pkg mgr to resolve some other location **in addition**, not instead of the 1st party pkgs?
PS: Is it time to merge in latest master to cut everything over to "modules"?
--
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
[GitHub] [solr] janhoy commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793141411
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -52,10 +52,17 @@
public class PackageLoader implements Closeable {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String LATEST = "$LATEST";
+ public static final String LPD = "solr.packages.local.dir";
+ public static final String SOLR_PACKAGES_LOCAL_ENABLED = "solr.packages.local.enabled";
+ public static final String LOCAL_PACKAGES_JSON = "local_packages.json";
+ public final String localPkgsDir = System.getProperty(LPD);
+ public final String localPkgsWhiteList = System.getProperty(SOLR_PACKAGES_LOCAL_ENABLED, "");
+ public final boolean enablePackages = Boolean.parseBoolean(System.getProperty("enable.packages", "false"));
Review comment:
Make a constant for the `enable.packages` property name, it is repeated many times in the code base.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +77,85 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
+ localPkgsDir :
+ coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!packagesPath.toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ File file = new File(packagesPath.toFile(), LOCAL_PACKAGES_JSON);
+ if(file.exists()) {
Review comment:
Please stop using the `File` api. Use `Path` and then `Files.exists(path)`
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +77,85 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
+ localPkgsDir :
+ coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!packagesPath.toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ File file = new File(packagesPath.toFile(), LOCAL_PACKAGES_JSON);
+ if(file.exists()) {
+ try {
+
+ try (InputStream in = new FileInputStream(file)) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+ } else {
+ //the no local_packages.json
+ //we will look for each subdirectory and consider them as a package
+ localPackages = readDirAsPackages(packagesPath);
+ }
+
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
+ }
+
+ /**If a directory with no local_packages.json is provided,
Review comment:
Start javadoc text on next line.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +77,85 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
Review comment:
You have one variable `enablePackages` and another `enabledPackages`. They are confusingly similar, perhaps choose `localPackagesToLoad` for the list?
Plese wrap the if body in `{ }` as a best practice even if only one statement.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +77,85 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
+ localPkgsDir :
+ coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
Review comment:
Use `Paths.get(coreContainer.getSolrHome()).resolve(localPkgsDir)` instead, which should result in the same.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +77,85 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
+ localPkgsDir :
+ coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!packagesPath.toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ File file = new File(packagesPath.toFile(), LOCAL_PACKAGES_JSON);
+ if(file.exists()) {
+ try {
+
+ try (InputStream in = new FileInputStream(file)) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+ } else {
+ //the no local_packages.json
+ //we will look for each subdirectory and consider them as a package
+ localPackages = readDirAsPackages(packagesPath);
+ }
+
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
+ }
+
+ /**If a directory with no local_packages.json is provided,
+ * each sub directory that contains one or more jar files
+ * will be treated as a package and each jar file in that
+ * directory is added to the package classpath
+ */
+ private PackageAPI.Packages readDirAsPackages(Path packagesPath) {
Review comment:
I cannot see a unit test for this method?
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestLocalPackages.java
##########
@@ -0,0 +1,145 @@
+package org.apache.solr.pkg;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.lang.invoke.MethodHandles;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.util.Utils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import static org.apache.solr.pkg.PackageLoader.LPD;
+import static org.apache.solr.pkg.PackageLoader.SOLR_PACKAGES_LOCAL_ENABLED;
+
+public class TestLocalPackages extends SolrCloudTestCase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ public void testLocalPackagesAsDir() throws Exception {
+ String PKG_NAME = "mypkg";
+ String jarName = "mypkg1.jar";
+ String COLLECTION_NAME = "testLocalPkgsColl";
+ String localPackagesDir = "testpkgdir";
+ System.setProperty(SOLR_PACKAGES_LOCAL_ENABLED, PKG_NAME);
+ System.setProperty(LPD, localPackagesDir);
+ MiniSolrCloudCluster cluster =
+ configureCluster(4)
+ .withJettyConfig(builder -> builder.enableV2(true))
+ .withJettyConfig(it -> it.withPreStartupHook(jsr -> {
+ try {
+ File pkgDir = new File(jsr.getSolrHome() + File.separator+ localPackagesDir);
+ if(!pkgDir.exists()) {
+ pkgDir.mkdir();
+ }
+ File subDir = new File(pkgDir, PKG_NAME);
+ if(!subDir.exists()) {
+ subDir.mkdir();
+ }
+ try (FileInputStream fis = new FileInputStream(getFile("runtimecode/runtimelibs.jar.bin"))) {
+ byte[] buf = new byte[fis.available()];
+
+ fis.read(buf);
+ try( FileOutputStream fos = new FileOutputStream( new File(subDir, jarName) )) {
+ fos.write(buf, 0,buf.length);
+ }
+ }
+
+ } catch (Exception e) {
+ throw new RuntimeException("Unable to create files", e);
+ }
+ }))
+ .addConfig("conf", configset("conf2"))
+ .configure();
+
+ System.clearProperty(SOLR_PACKAGES_LOCAL_ENABLED);
+ System.clearProperty(LPD);
+ try {
+ for (JettySolrRunner jsr : cluster.getJettySolrRunners()) {
+ List<String> packageFiles = Arrays.asList(new File(jsr.getSolrHome()+File.separator+localPackagesDir + File.separator+PKG_NAME).list());
+ assertTrue(packageFiles.contains(jarName));
+ }
+ CollectionAdminRequest
+ .createCollection(COLLECTION_NAME, "conf", 2, 2)
+ .process(cluster.getSolrClient());
+ cluster.waitForActiveCollection(COLLECTION_NAME, 2, 4);
+
+ log.info("Collection created successfully");
+
+ TestPackages.verifyComponent(cluster.getSolrClient(), COLLECTION_NAME, "query", "filterCache", PKG_NAME ,"0" );
+ } finally {
+ cluster.shutdown();
+ }
+ }
+
+ public void testLocalPackages() throws Exception {
+ String PKG_NAME = "mypkg";
+ String jarName = "mypkg1.jar";
+ String COLLECTION_NAME = "testLocalPkgsColl";
+ String localPackagesDir = "local_packages";
+ PackageAPI.Packages p = new PackageAPI.Packages();
+ PackageAPI.PkgVersion pkgVersion = new PackageAPI.PkgVersion();
+ pkgVersion.files = Collections.singletonList(jarName);
+ pkgVersion.version = "0.1";
+ pkgVersion.pkg = PKG_NAME;
+ p.packages.put(PKG_NAME, Collections.singletonList(pkgVersion));
+
+ log.info("local_packages.json: " + Utils.toJSONString(p));
+ log.info("Local packages dir: " + localPackagesDir);
+ System.setProperty(SOLR_PACKAGES_LOCAL_ENABLED, PKG_NAME);
+ System.setProperty(LPD, localPackagesDir);
+ MiniSolrCloudCluster cluster =
Review comment:
Wrap cluster creation in a try-finally block?
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -274,20 +349,25 @@ public void writeMap(EntryWriter ew) throws IOException {
version.writeMap(ew);
}
- Version(Package parent, PackageAPI.PkgVersion v) {
+ Version(Package parent, PackageAPI.PkgVersion v, Path localPkgDir) {
this.parent = parent;
this.version = v;
List<Path> paths = new ArrayList<>();
-
- List<String> errs = new ArrayList<>();
- coreContainer.getPackageStoreAPI().validateFiles(version.files, true, s -> errs.add(s));
- if(!errs.isEmpty()) {
- throw new RuntimeException("Cannot load package: " +errs);
- }
- for (String file : version.files) {
- paths.add(coreContainer.getPackageStoreAPI().getPackageStore().getRealpath(file));
+ if(localPkgDir != null) {
+ for (String file : v.files) {
+ if(file.charAt(0)== '/') file =file.substring(1);
+ paths.add( localPkgDir.resolve(file).toAbsolutePath()) ;
+ }
+ } else {
Review comment:
Some code smell here with if-then-else based on quite different behaviour - local packages that are just added t path vs remote packages that need validation etc. Can we have sublcassing instead, either `ValidatingPackageLoader` and `LocalPackageLoader`, and the same for Package.Version?
--
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
[GitHub] [solr] noblepaul commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793269001
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +77,85 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
+ localPkgsDir :
+ coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!packagesPath.toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ File file = new File(packagesPath.toFile(), LOCAL_PACKAGES_JSON);
+ if(file.exists()) {
+ try {
+
+ try (InputStream in = new FileInputStream(file)) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+ } else {
+ //the no local_packages.json
+ //we will look for each subdirectory and consider them as a package
+ localPackages = readDirAsPackages(packagesPath);
+ }
+
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
+ }
+
+ /**If a directory with no local_packages.json is provided,
+ * each sub directory that contains one or more jar files
+ * will be treated as a package and each jar file in that
+ * directory is added to the package classpath
+ */
+ private PackageAPI.Packages readDirAsPackages(Path packagesPath) {
Review comment:
`TestLocalPackages#testLocalPackagesAsDir()`
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r792884314
##########
File path: solr/contrib/scripting/manifest.json
##########
@@ -0,0 +1,22 @@
+{
+ "version-constraint": "10",
Review comment:
What is the point of version constraints for local packages?
Can this be removed altogether? If not, then it should be autogenerated by gradle when copying over to the `build/packaging` directory. It should not be hardcoded.
--
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
[GitHub] [solr] janhoy commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r792755979
##########
File path: solr/contrib/extraction/manifest.json
##########
@@ -0,0 +1,21 @@
+{
+ "version-constraint": "9",
+ "plugins": [
+ {
+ "name": "update-extraction",
+ "setup-command": {
+ "path": "/api/collections/${collection}/config",
+ "payload": {"add-requesthandler": {"name": "${NAME}", "class": "solr-extraction:org.apache.solr.handler.extraction.ExtractingRequestHandler"}},
Review comment:
Why is the class-name prefix `solr-extraction:` while the plugin name is `update-extraction`? What is the link?
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r792881998
##########
File path: gradle/solr/packaging.gradle
##########
@@ -60,7 +60,11 @@ configure(allprojects.findAll {project -> project.path.startsWith(":solr:contrib
// An aggregate that configures lib and test-lib in a temporary location.
task assemblePackaging(type: Sync) {
from "README.md"
-
+
+ from "manifest.json"
+
+ from "src/resources/solr-default-tika-config.xml"
Review comment:
I removed this. The `solr-default-tika-config.xml` is already included in the JAR, given that it's in `src/resources`
--
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
[GitHub] [solr] janhoy commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793003077
##########
File path: solr/contrib/scripting/manifest.json
##########
@@ -0,0 +1,22 @@
+{
+ "version-constraint": "10",
Review comment:
Agreed. Local package bug fixes are always released together with Solr, so you'd never want to, say, run Solr 9.0.0 with a LTR package 9.1.0. Makes no sense. So I think we could just remove version-constraint for these manifests, if that it allowed.
--
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
[GitHub] [solr] noblepaul commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793301928
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestLocalPackages.java
##########
@@ -0,0 +1,101 @@
+package org.apache.solr.pkg;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.lang.invoke.MethodHandles;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.util.Utils;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TestLocalPackages extends SolrCloudTestCase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static String localPackagesDir = getFile("runtimecode").getAbsolutePath();
+ public static String PKG_NAME = "mypkg";
+ public static String jarName = "mypkg1.jar";
+ public static String COLLECTION_NAME = "testLocalPkgsColl";
+
+ @BeforeClass
+ public static void setup() {
+ System.setProperty("solr.packages.local.whitelist", PKG_NAME);
+ System.setProperty("solr.packages.local.dir", localPackagesDir);
+ }
+
+ @AfterClass
+ public static void shutdown() {
+ System.clearProperty("solr.packages.local.whitelist");
+ System.clearProperty("solr.packages.local.dir");
+ }
+
+ public void testLocalPackages() throws Exception {
+
+ PackageAPI.Packages p = new PackageAPI.Packages();
+ PackageAPI.PkgVersion pkgVersion = new PackageAPI.PkgVersion();
+ pkgVersion.files = Collections.singletonList(jarName);
+ pkgVersion.version = "0.1";
+ pkgVersion.pkg = PKG_NAME;
+ p.packages.put(PKG_NAME, Collections.singletonList(pkgVersion));
+
+ log.info("local_packages.json: " + Utils.toJSONString(p));
+ log.info("Local packages dir: " + localPackagesDir);
+
+ MiniSolrCloudCluster cluster =
+ configureCluster(4)
+ .withJettyConfig(builder -> builder.enableV2(true))
+ .withJettyConfig(it -> it.withPreStartupHook(jsr -> {
Review comment:
> I'm curious; why is this needed vs simply calling this code before cluster is instantiated?
We need this code to be executed after the `JettySolrRunner` is created but before the jetty is started because the paths are only available at that point.
> I think it's better style to put all this code into a method and simply call it rather than have a long lambda.
👍
--
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
[GitHub] [solr] chatman commented on pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
chatman commented on pull request #560:
URL: https://github.com/apache/solr/pull/560#issuecomment-1027852600
@HoustonPutman I tried to merge main into this branch, rebase on main, as well as apply these changes on a fresh branch forked from main.
On every occasion, the doLast task in solr/modules/build.gradle is not executing at all, and hence the local_packages.json isn't getting generated. Here's a branch based on fresh main. https://github.com/apache/solr/tree/jira/solr-15951. Any idea, please?
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r795322626
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,9 +82,112 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(enabledLocalPkgsList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(localPkgsDir, enabledPackages);
+ }
+ if (repoPackagesEnabled) {
+ refreshPackageConf();
+ }
+ }
+
+ private void loadLocalPackages(String localPkgsDir, List<String> enabledPackages) {
+ final Path packagesPath = localPkgsDir.charAt(0) == File.separatorChar ?
+ Paths.get(localPkgsDir) :
+ Paths.get(coreContainer.getSolrHome()).resolve(localPkgsDir);
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!Files.exists(packagesPath)) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ Path packagesJsonPath = packagesPath.resolve(LOCAL_PACKAGES_JSON);
+ if(Files.exists(packagesJsonPath)) {
+ try {
+
+ try (InputStream in = Files.newInputStream(packagesJsonPath)) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+ } else {
+ //the no local_packages.json
+ //we will look for each subdirectory and consider them as a package
+ localPackages = readDirAsPackages(packagesPath);
+ }
+
+ if(localPackages != null) {
+ localPackages.packages.forEach((s, versions) -> versions.forEach(v -> {
+ v.local = Boolean.TRUE;
+ }));
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey(), e.getValue());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
+ }
+ }
+
+ /**
+ * If a directory with no local_packages.json is provided,
+ * each sub directory that contains one or more jar files
+ * will be treated as a package and each jar file in that
+ * directory is added to the package classpath
+ */
+ private PackageAPI.Packages readDirAsPackages(Path packagesPath) {
+ PackageAPI.Packages localDirAsPackages = new PackageAPI.Packages();
+ try {
+ Files.list(packagesPath).forEach(subDir -> {
+ if (Files.isDirectory(subDir)) {
+ PackageAPI.PkgVersion version = new PackageAPI.PkgVersion();
+ version.pkg = subDir.getFileName().toString();
+ version.version = "0";
+ version.files = new ArrayList<>();
+ try {
+ Files.list(subDir).forEach(file -> {
Review comment:
*StreamResourceLeak:* Streams that encapsulate a closeable resource should be closed using try-with-resources [(details)](https://errorprone.info/bugpattern/StreamResourceLeak)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,9 +82,112 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(enabledLocalPkgsList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(localPkgsDir, enabledPackages);
+ }
+ if (repoPackagesEnabled) {
+ refreshPackageConf();
+ }
+ }
+
+ private void loadLocalPackages(String localPkgsDir, List<String> enabledPackages) {
+ final Path packagesPath = localPkgsDir.charAt(0) == File.separatorChar ?
+ Paths.get(localPkgsDir) :
+ Paths.get(coreContainer.getSolrHome()).resolve(localPkgsDir);
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!Files.exists(packagesPath)) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ Path packagesJsonPath = packagesPath.resolve(LOCAL_PACKAGES_JSON);
+ if(Files.exists(packagesJsonPath)) {
+ try {
+
+ try (InputStream in = Files.newInputStream(packagesJsonPath)) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+ } else {
+ //the no local_packages.json
+ //we will look for each subdirectory and consider them as a package
+ localPackages = readDirAsPackages(packagesPath);
+ }
+
+ if(localPackages != null) {
+ localPackages.packages.forEach((s, versions) -> versions.forEach(v -> {
+ v.local = Boolean.TRUE;
+ }));
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey(), e.getValue());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
+ }
+ }
+
+ /**
+ * If a directory with no local_packages.json is provided,
+ * each sub directory that contains one or more jar files
+ * will be treated as a package and each jar file in that
+ * directory is added to the package classpath
+ */
+ private PackageAPI.Packages readDirAsPackages(Path packagesPath) {
+ PackageAPI.Packages localDirAsPackages = new PackageAPI.Packages();
+ try {
+ Files.list(packagesPath).forEach(subDir -> {
Review comment:
*StreamResourceLeak:* Streams that encapsulate a closeable resource should be closed using try-with-resources [(details)](https://errorprone.info/bugpattern/StreamResourceLeak)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r795317769
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -160,16 +247,22 @@ public void notifyListeners(String pkg) {
/**
* represents a package definition in the packages.json
*/
- public class Package implements Closeable {
+ public class Package implements Closeable, MapWriter {
Review comment:
*JavaLangClash:* org.apache.solr.pkg.PackageLoader.Package clashes with java.lang.Package [(details)](https://errorprone.info)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -181,14 +274,16 @@ public boolean isDeleted() {
}
- private synchronized void updateVersions(List<PackageAPI.PkgVersion> modified) {
+ private synchronized void updateVersions(List<PackageAPI.PkgVersion> modified, Path localpkgDir) {
for (PackageAPI.PkgVersion v : modified) {
Version version = myVersions.get(v.version);
if (version == null) {
log.info("A new version: {} added for package: {} with artifacts {}", v.version, this.name, v.files);
Version ver = null;
try {
- ver = new Version(this, v);
+ ver = v.local == Boolean.TRUE ?
Review comment:
*ReferenceEquality:* Comparison using reference equality instead of value equality [(details)](https://errorprone.info/bugpattern/ReferenceEquality)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] madrob commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793065102
##########
File path: solr/contrib/build.gradle
##########
@@ -0,0 +1,77 @@
+import groovy.json.JsonOutput
+import org.gradle.api.internal.artifacts.dependencies.DefaultProjectDependency
Review comment:
unused?
##########
File path: solr/contrib/build.gradle
##########
@@ -0,0 +1,77 @@
+import groovy.json.JsonOutput
+import org.gradle.api.internal.artifacts.dependencies.DefaultProjectDependency
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+description = 'Solr Modules'
+
+ext {
+ packagingDir = file("$buildDir/packaging")
+ localPackagesFile = "$packagingDir/local_packages.json"
+}
+
+configurations {
+ modulePackages
+ packaging
+}
+
+dependencies {
+ [":solr:contrib:extraction",
+ ":solr:contrib:scripting"
+ ].each { moduleName ->
+ modulePackages project(path: moduleName, configuration: "packaging")
+ }
+
+ packaging files(packagingDir) {
+ builtBy 'createLocalPackagesJson'
+ }
+}
+
+task createLocalPackagesJson {
+ dependsOn configurations.modulePackages
+
+ doLast {
+ def packagesJson = [
+ packages: [:]
+ ]
+ project(":solr:contrib:scripting").configurations.packaging.resolve().each {
+ logger.error("It: $it")
+ }
Review comment:
vestigal logging?
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r792307699
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -56,9 +52,12 @@
public class PackageLoader implements Closeable {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
public static final String LATEST = "$LATEST";
- public static final String LOCAL_PACKAGES_DIR = System.getProperty("solr.packages.local.dir");
- public static final String LOCAL_PACKAGES_WHITELIST = System.getProperty("solr.packages.local.whitelist", "");
+ public static final String LPD = "solr.packages.local.dir";
Review comment:
Expand the acronym please. I didn't know what it was at the spot that referenced it.
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestLocalPackages.java
##########
@@ -12,33 +12,75 @@
import org.apache.solr.cloud.MiniSolrCloudCluster;
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.util.Utils;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.solr.pkg.PackageLoader.LPD;
+import static org.apache.solr.pkg.PackageLoader.SOLR_PACKAGES_LOCAL_ENABLED;
+
public class TestLocalPackages extends SolrCloudTestCase {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ public void testLocalPackagesAsDir() throws Exception {
+ String PKG_NAME = "mypkg";
+ String jarName = "mypkg1.jar";
+ String COLLECTION_NAME = "testLocalPkgsColl";
+ String localPackagesDir = "testpkgdir";
+ System.setProperty(SOLR_PACKAGES_LOCAL_ENABLED, PKG_NAME);
+ System.setProperty(LPD, localPackagesDir);
+ MiniSolrCloudCluster cluster =
+ configureCluster(4)
Review comment:
Do we need a cluster this big to test this?
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -115,6 +120,36 @@ private void loadLocalPackages(List<String> enabledPackages) {
}
}
+ /**If a directory with no local_packages.json is provided,
+ * each sub directory that contains one or more jar files
+ * will be treated as a package and each jar file in that
+ * directory is added to the package classpath
+ */
+ private PackageAPI.Packages readDirAsPackages(Path packagesPath) {
+ PackageAPI.Packages localDirAsPackages = new PackageAPI.Packages();
+ packagesPath.toFile().list((dir, pkgName) -> {
+ File subDir = new File(dir, pkgName);
Review comment:
Let's not use java.io.File and instead use Path, wherever possible.
--
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
[GitHub] [solr] janhoy commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793333070
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -78,29 +82,31 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
- List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
+ List<String> enabledPackages = StrUtils.splitSmart(enabledLocalPkgsList, ',');
packageAPI = new PackageAPI(coreContainer, this);
if (localPkgsDir != null && !enabledPackages.isEmpty()) {
- loadLocalPackages(enabledPackages);
+ loadLocalPackages(localPkgsDir, enabledPackages);
+ }
+ if (repoPackagesEnabled) {
+ refreshPackageConf();
}
- if (enablePackages) refreshPackageConf();
}
- private void loadLocalPackages(List<String> enabledPackages) {
- final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
- localPkgsDir :
- coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
+ private void loadLocalPackages(String localPkgsDir, List<String> enabledPackages) {
+ final Path packagesPath = localPkgsDir.charAt(0) == File.separatorChar ?
+ Paths.get(localPkgsDir) :
+ Paths.get(coreContainer.getSolrHome()).resolve(localPkgsDir);
log.info("Packages to be loaded from directory: {}", packagesPath);
- if (!packagesPath.toFile().exists()) {
+ if (!Files.exists(packagesPath)) {
Review comment:
Is there or will there be another way than sysProp to set the local packages dir? If it can be set by some api, it should be checked against allowPaths in cc.
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r797814806
##########
File path: solr/bin/solr
##########
@@ -2264,6 +2264,7 @@ function start_solr() {
# users who don't care about useful error msgs can override in SOLR_OPTS with +OmitStackTraceInFastThrow
"${SOLR_HOST_ARG[@]}" "-Duser.timezone=$SOLR_TIMEZONE" "-XX:-OmitStackTraceInFastThrow" \
"-XX:OnOutOfMemoryError=$SOLR_TIP/bin/oom_solr.sh $SOLR_PORT $SOLR_LOGS_DIR" \
+ "-Dsolr.packages.local.dir=$SOLR_TIP/contrib" \
Review comment:
So I agree that this likely doesn't need to be customizable. But if we intend to allow customization, how about we just provide this as a default when reading in the value? (And also read from both sysProp as well as envVar). That way there's no need to modify the bin/solr or bin/solr.cmd
--
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
[GitHub] [solr] chatman commented on pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
chatman commented on pull request #560:
URL: https://github.com/apache/solr/pull/560#issuecomment-1025351592
@HoustonPutman Gradle changes look and work great :-)
--
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
[GitHub] [solr] HoustonPutman commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793098853
##########
File path: solr/contrib/build.gradle
##########
@@ -0,0 +1,77 @@
+import groovy.json.JsonOutput
+import org.gradle.api.internal.artifacts.dependencies.DefaultProjectDependency
Review comment:
yeah forgot to remove all the debugging stuff.
--
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
[GitHub] [solr] janhoy commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793487654
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +77,85 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
+ localPkgsDir :
+ coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!packagesPath.toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ File file = new File(packagesPath.toFile(), LOCAL_PACKAGES_JSON);
+ if(file.exists()) {
+ try {
+
+ try (InputStream in = new FileInputStream(file)) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+ } else {
+ //the no local_packages.json
+ //we will look for each subdirectory and consider them as a package
+ localPackages = readDirAsPackages(packagesPath);
+ }
+
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
+ }
+
+ /**If a directory with no local_packages.json is provided,
+ * each sub directory that contains one or more jar files
+ * will be treated as a package and each jar file in that
+ * directory is added to the package classpath
+ */
+ private PackageAPI.Packages readDirAsPackages(Path packagesPath) {
Review comment:
Well, that is a happy-path integration test. I more thought of a unit test that simply creates a tmpDirectory folder with several different (and erraneous) sub folders inside to validate that you get exactley the expected packages back, and that failure cases are handled correctly.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r791227809
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestLocalPackages.java
##########
@@ -0,0 +1,101 @@
+package org.apache.solr.pkg;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.lang.invoke.MethodHandles;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.util.Utils;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TestLocalPackages extends SolrCloudTestCase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static String localPackagesDir = getFile("runtimecode").getAbsolutePath();
+ public static String PKG_NAME = "mypkg";
+ public static String jarName = "mypkg1.jar";
+ public static String COLLECTION_NAME = "testLocalPkgsColl";
+
+ @BeforeClass
+ public static void setup() {
+ System.setProperty("solr.packages.local.whitelist", PKG_NAME);
+ System.setProperty("solr.packages.local.dir", localPackagesDir);
+ }
+
+ @AfterClass
+ public static void shutdown() {
+ System.clearProperty("solr.packages.local.whitelist");
+ System.clearProperty("solr.packages.local.dir");
+ }
+
+ public void testLocalPackages() throws Exception {
+
+ PackageAPI.Packages p = new PackageAPI.Packages();
+ PackageAPI.PkgVersion pkgVersion = new PackageAPI.PkgVersion();
+ pkgVersion.files = Collections.singletonList(jarName);
+ pkgVersion.version = "0.1";
+ pkgVersion.pkg = PKG_NAME;
+ p.packages.put(PKG_NAME, Collections.singletonList(pkgVersion));
+
+ log.info("local_packages.json: " + Utils.toJSONString(p));
+ log.info("Local packages dir: " + localPackagesDir);
+
+ MiniSolrCloudCluster cluster =
+ configureCluster(4)
+ .withJettyConfig(builder -> builder.enableV2(true))
+ .withJettyConfig(it -> it.withPreStartupHook(jsr -> {
Review comment:
I'm curious; why is this needed vs simply calling this code before cluster is instantiated?
Also, I think it's better style to put all this code into a method and simply call it rather than have a long lambda.
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestLocalPackages.java
##########
@@ -0,0 +1,101 @@
+package org.apache.solr.pkg;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.lang.invoke.MethodHandles;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.util.Utils;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TestLocalPackages extends SolrCloudTestCase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static String localPackagesDir = getFile("runtimecode").getAbsolutePath();
+ public static String PKG_NAME = "mypkg";
+ public static String jarName = "mypkg1.jar";
+ public static String COLLECTION_NAME = "testLocalPkgsColl";
+
+ @BeforeClass
+ public static void setup() {
+ System.setProperty("solr.packages.local.whitelist", PKG_NAME);
+ System.setProperty("solr.packages.local.dir", localPackagesDir);
+ }
+
+ @AfterClass
+ public static void shutdown() {
+ System.clearProperty("solr.packages.local.whitelist");
+ System.clearProperty("solr.packages.local.dir");
+ }
+
+ public void testLocalPackages() throws Exception {
+
+ PackageAPI.Packages p = new PackageAPI.Packages();
+ PackageAPI.PkgVersion pkgVersion = new PackageAPI.PkgVersion();
+ pkgVersion.files = Collections.singletonList(jarName);
+ pkgVersion.version = "0.1";
+ pkgVersion.pkg = PKG_NAME;
+ p.packages.put(PKG_NAME, Collections.singletonList(pkgVersion));
+
+ log.info("local_packages.json: " + Utils.toJSONString(p));
+ log.info("Local packages dir: " + localPackagesDir);
+
+ MiniSolrCloudCluster cluster =
+ configureCluster(4)
+ .withJettyConfig(builder -> builder.enableV2(true))
+ .withJettyConfig(it -> it.withPreStartupHook(jsr -> {
+ try {
+ File pkgDir = new File(localPackagesDir);
+ if(!pkgDir.exists()) {
+ pkgDir.mkdir();
+ }
+ try (FileInputStream fis = new FileInputStream(getFile("runtimecode/runtimelibs.jar.bin"))) {
Review comment:
I could be wrong but I bet there are simpler / more concise idioms available to us in Java 11 to do this basic task.
--
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
[GitHub] [solr] dsmiley commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r791227809
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestLocalPackages.java
##########
@@ -0,0 +1,101 @@
+package org.apache.solr.pkg;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.lang.invoke.MethodHandles;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.util.Utils;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TestLocalPackages extends SolrCloudTestCase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static String localPackagesDir = getFile("runtimecode").getAbsolutePath();
+ public static String PKG_NAME = "mypkg";
+ public static String jarName = "mypkg1.jar";
+ public static String COLLECTION_NAME = "testLocalPkgsColl";
+
+ @BeforeClass
+ public static void setup() {
+ System.setProperty("solr.packages.local.whitelist", PKG_NAME);
+ System.setProperty("solr.packages.local.dir", localPackagesDir);
+ }
+
+ @AfterClass
+ public static void shutdown() {
+ System.clearProperty("solr.packages.local.whitelist");
+ System.clearProperty("solr.packages.local.dir");
+ }
+
+ public void testLocalPackages() throws Exception {
+
+ PackageAPI.Packages p = new PackageAPI.Packages();
+ PackageAPI.PkgVersion pkgVersion = new PackageAPI.PkgVersion();
+ pkgVersion.files = Collections.singletonList(jarName);
+ pkgVersion.version = "0.1";
+ pkgVersion.pkg = PKG_NAME;
+ p.packages.put(PKG_NAME, Collections.singletonList(pkgVersion));
+
+ log.info("local_packages.json: " + Utils.toJSONString(p));
+ log.info("Local packages dir: " + localPackagesDir);
+
+ MiniSolrCloudCluster cluster =
+ configureCluster(4)
+ .withJettyConfig(builder -> builder.enableV2(true))
+ .withJettyConfig(it -> it.withPreStartupHook(jsr -> {
Review comment:
I'm curious; why is this needed vs simply calling this code before cluster is instantiated?
Also, I think it's better style to put all this code into a method and simply call it rather than have a long lambda.
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestLocalPackages.java
##########
@@ -0,0 +1,101 @@
+package org.apache.solr.pkg;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.lang.invoke.MethodHandles;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.util.Utils;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TestLocalPackages extends SolrCloudTestCase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static String localPackagesDir = getFile("runtimecode").getAbsolutePath();
+ public static String PKG_NAME = "mypkg";
+ public static String jarName = "mypkg1.jar";
+ public static String COLLECTION_NAME = "testLocalPkgsColl";
+
+ @BeforeClass
+ public static void setup() {
+ System.setProperty("solr.packages.local.whitelist", PKG_NAME);
+ System.setProperty("solr.packages.local.dir", localPackagesDir);
+ }
+
+ @AfterClass
+ public static void shutdown() {
+ System.clearProperty("solr.packages.local.whitelist");
+ System.clearProperty("solr.packages.local.dir");
+ }
+
+ public void testLocalPackages() throws Exception {
+
+ PackageAPI.Packages p = new PackageAPI.Packages();
+ PackageAPI.PkgVersion pkgVersion = new PackageAPI.PkgVersion();
+ pkgVersion.files = Collections.singletonList(jarName);
+ pkgVersion.version = "0.1";
+ pkgVersion.pkg = PKG_NAME;
+ p.packages.put(PKG_NAME, Collections.singletonList(pkgVersion));
+
+ log.info("local_packages.json: " + Utils.toJSONString(p));
+ log.info("Local packages dir: " + localPackagesDir);
+
+ MiniSolrCloudCluster cluster =
+ configureCluster(4)
+ .withJettyConfig(builder -> builder.enableV2(true))
+ .withJettyConfig(it -> it.withPreStartupHook(jsr -> {
+ try {
+ File pkgDir = new File(localPackagesDir);
+ if(!pkgDir.exists()) {
+ pkgDir.mkdir();
+ }
+ try (FileInputStream fis = new FileInputStream(getFile("runtimecode/runtimelibs.jar.bin"))) {
Review comment:
I could be wrong but I bet there are simpler / more concise idioms available to us in Java 11 to do this basic task.
--
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
[GitHub] [solr] noblepaul commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793327761
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestLocalPackages.java
##########
@@ -0,0 +1,101 @@
+package org.apache.solr.pkg;
+
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.lang.invoke.MethodHandles;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import org.apache.solr.client.solrj.embedded.JettySolrRunner;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.MiniSolrCloudCluster;
+import org.apache.solr.cloud.SolrCloudTestCase;
+import org.apache.solr.common.util.Utils;
+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class TestLocalPackages extends SolrCloudTestCase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static String localPackagesDir = getFile("runtimecode").getAbsolutePath();
+ public static String PKG_NAME = "mypkg";
+ public static String jarName = "mypkg1.jar";
+ public static String COLLECTION_NAME = "testLocalPkgsColl";
+
+ @BeforeClass
+ public static void setup() {
+ System.setProperty("solr.packages.local.whitelist", PKG_NAME);
+ System.setProperty("solr.packages.local.dir", localPackagesDir);
+ }
+
+ @AfterClass
+ public static void shutdown() {
+ System.clearProperty("solr.packages.local.whitelist");
+ System.clearProperty("solr.packages.local.dir");
+ }
+
+ public void testLocalPackages() throws Exception {
+
+ PackageAPI.Packages p = new PackageAPI.Packages();
+ PackageAPI.PkgVersion pkgVersion = new PackageAPI.PkgVersion();
+ pkgVersion.files = Collections.singletonList(jarName);
+ pkgVersion.version = "0.1";
+ pkgVersion.pkg = PKG_NAME;
+ p.packages.put(PKG_NAME, Collections.singletonList(pkgVersion));
+
+ log.info("local_packages.json: " + Utils.toJSONString(p));
+ log.info("Local packages dir: " + localPackagesDir);
+
+ MiniSolrCloudCluster cluster =
+ configureCluster(4)
+ .withJettyConfig(builder -> builder.enableV2(true))
+ .withJettyConfig(it -> it.withPreStartupHook(jsr -> {
+ try {
+ File pkgDir = new File(localPackagesDir);
+ if(!pkgDir.exists()) {
+ pkgDir.mkdir();
+ }
+ try (FileInputStream fis = new FileInputStream(getFile("runtimecode/runtimelibs.jar.bin"))) {
Review comment:
sure. As it was a test, I didn't explore the idiomatic way of doing this
##########
File path: solr/core/src/test/org/apache/solr/pkg/TestLocalPackages.java
##########
@@ -12,33 +12,75 @@
import org.apache.solr.cloud.MiniSolrCloudCluster;
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.util.Utils;
-import org.junit.AfterClass;
-import org.junit.BeforeClass;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.solr.pkg.PackageLoader.LPD;
+import static org.apache.solr.pkg.PackageLoader.SOLR_PACKAGES_LOCAL_ENABLED;
+
public class TestLocalPackages extends SolrCloudTestCase {
private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ public void testLocalPackagesAsDir() throws Exception {
+ String PKG_NAME = "mypkg";
+ String jarName = "mypkg1.jar";
+ String COLLECTION_NAME = "testLocalPkgsColl";
+ String localPackagesDir = "testpkgdir";
+ System.setProperty(SOLR_PACKAGES_LOCAL_ENABLED, PKG_NAME);
+ System.setProperty(LPD, localPackagesDir);
+ MiniSolrCloudCluster cluster =
+ configureCluster(4)
Review comment:
2 should be fine
--
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
[GitHub] [solr] HoustonPutman commented on pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #560:
URL: https://github.com/apache/solr/pull/560#issuecomment-1028130680
Ok I merged `main` in and got stuff working. Haven't really tested everything, but I did test that the packaging works, and the the `local_packages.json` and the `manifest.json` files get created.
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r805289794
##########
File path: solr/core/src/java/org/apache/solr/util/PackageTool.java
##########
@@ -105,12 +120,38 @@ protected void runImpl(CommandLine cli) throws Exception {
break;
case "list-available":
PackageUtils.printGreen("Available packages:\n-----");
+
+
+ AsciiTable table = new AsciiTable();
+ table.addRule();
+ table.addRow("Name", "Description", "Versions", "Repository");
+ table.addRule();
+
+ SolrResponse resp = new V2Request.Builder("/node/packages")
+ .withMethod(SolrRequest.METHOD.GET)
+ .build().process(cloudSolrClient);
+ Map<String, Object> local = (Map<String, Object>) new ObjectMapper().readValue(resp.jsonStr(), Map.class);
+ local = (Map<String, Object>) local.get("packages");
+ for (String pkg: local.keySet()) {
+ String versions = "";
+ for (Map<String, Object> version: ((List<Map<String, Object>>) ((Map<String, Object>) local.get(pkg)).get("versions"))) {
Review comment:
*INEFFICIENT_KEYSET_ITERATOR:* Accessing a value using a key that was retrieved from a `keySet` iterator. It is more efficient to use an iterator on the `entrySet` of the map, avoiding the extra `HashMap.get(key)` lookup.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r805289794
##########
File path: solr/core/src/java/org/apache/solr/util/PackageTool.java
##########
@@ -105,12 +120,38 @@ protected void runImpl(CommandLine cli) throws Exception {
break;
case "list-available":
PackageUtils.printGreen("Available packages:\n-----");
+
+
+ AsciiTable table = new AsciiTable();
+ table.addRule();
+ table.addRow("Name", "Description", "Versions", "Repository");
+ table.addRule();
+
+ SolrResponse resp = new V2Request.Builder("/node/packages")
+ .withMethod(SolrRequest.METHOD.GET)
+ .build().process(cloudSolrClient);
+ Map<String, Object> local = (Map<String, Object>) new ObjectMapper().readValue(resp.jsonStr(), Map.class);
+ local = (Map<String, Object>) local.get("packages");
+ for (String pkg: local.keySet()) {
+ String versions = "";
+ for (Map<String, Object> version: ((List<Map<String, Object>>) ((Map<String, Object>) local.get(pkg)).get("versions"))) {
Review comment:
*INEFFICIENT_KEYSET_ITERATOR:* Accessing a value using a key that was retrieved from a `keySet` iterator. It is more efficient to use an iterator on the `entrySet` of the map, avoiding the extra `HashMap.get(key)` lookup.
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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
[GitHub] [solr] chatman commented on pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
chatman commented on pull request #560:
URL: https://github.com/apache/solr/pull/560#issuecomment-1025351025
I had some issues rebasing to latest main around the gradle part (due to the "contrib" vs "modules" switch). I'll give it a try again shortly. FYI @noblepaul.
--
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
[GitHub] [solr] janhoy commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r793487654
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +77,85 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
+ localPkgsDir :
+ coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!packagesPath.toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ File file = new File(packagesPath.toFile(), LOCAL_PACKAGES_JSON);
+ if(file.exists()) {
+ try {
+
+ try (InputStream in = new FileInputStream(file)) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+ } else {
+ //the no local_packages.json
+ //we will look for each subdirectory and consider them as a package
+ localPackages = readDirAsPackages(packagesPath);
+ }
+
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
+ }
+
+ /**If a directory with no local_packages.json is provided,
+ * each sub directory that contains one or more jar files
+ * will be treated as a package and each jar file in that
+ * directory is added to the package classpath
+ */
+ private PackageAPI.Packages readDirAsPackages(Path packagesPath) {
Review comment:
Well, that is a happy-path integration test. I more thought of a unit test that simply creates a tmpDirectory folder with several different (and some erraneous) sub folders inside to validate that you get exactley the expected packages back, and that failure cases are handled correctly.
--
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
[GitHub] [solr] noblepaul commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r794187077
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -70,17 +77,85 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
+
+ List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
packageAPI = new PackageAPI(coreContainer, this);
- refreshPackageConf();
+ if (localPkgsDir != null && !enabledPackages.isEmpty()) {
+ loadLocalPackages(enabledPackages);
+ }
+ if (enablePackages) refreshPackageConf();
+ }
+
+ private void loadLocalPackages(List<String> enabledPackages) {
+ final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
+ localPkgsDir :
+ coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
+ log.info("Packages to be loaded from directory: {}", packagesPath);
+
+ if (!packagesPath.toFile().exists()) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "No such directory: " + packagesPath);
+ }
+ File file = new File(packagesPath.toFile(), LOCAL_PACKAGES_JSON);
+ if(file.exists()) {
+ try {
+
+ try (InputStream in = new FileInputStream(file)) {
+ localPackages = PackageAPI.mapper.readValue(in, PackageAPI.Packages.class);
+ }
+ } catch (IOException e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, "Error reading local_packages.json", e);
+ }
+ } else {
+ //the no local_packages.json
+ //we will look for each subdirectory and consider them as a package
+ localPackages = readDirAsPackages(packagesPath);
+ }
+
+ for (Map.Entry<String, List<PackageAPI.PkgVersion>> e : localPackages.packages.entrySet()) {
+ if(!enabledPackages.contains(e.getKey())) continue;
+ Package p = new Package(e.getKey());
+ p.updateVersions(e.getValue(), packagesPath);
+ packageClassLoaders.put(e.getKey(), p);
+ }
+ }
+
+ /**If a directory with no local_packages.json is provided,
+ * each sub directory that contains one or more jar files
+ * will be treated as a package and each jar file in that
+ * directory is added to the package classpath
+ */
+ private PackageAPI.Packages readDirAsPackages(Path packagesPath) {
Review comment:
I shall add one. but the code is written to handle directories with jars only
--
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
[GitHub] [solr] noblepaul commented on a change in pull request #560: SOLR-15951: Local packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #560:
URL: https://github.com/apache/solr/pull/560#discussion_r794186760
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -78,29 +82,31 @@
public PackageLoader(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
- List<String> enabledPackages = StrUtils.splitSmart(localPkgsWhiteList, ',');
+ List<String> enabledPackages = StrUtils.splitSmart(enabledLocalPkgsList, ',');
packageAPI = new PackageAPI(coreContainer, this);
if (localPkgsDir != null && !enabledPackages.isEmpty()) {
- loadLocalPackages(enabledPackages);
+ loadLocalPackages(localPkgsDir, enabledPackages);
+ }
+ if (repoPackagesEnabled) {
+ refreshPackageConf();
}
- if (enablePackages) refreshPackageConf();
}
- private void loadLocalPackages(List<String> enabledPackages) {
- final Path packagesPath = new File(localPkgsDir.charAt(0)== File.separatorChar?
- localPkgsDir :
- coreContainer.getSolrHome()+ File.separator+ localPkgsDir).toPath();
+ private void loadLocalPackages(String localPkgsDir, List<String> enabledPackages) {
+ final Path packagesPath = localPkgsDir.charAt(0) == File.separatorChar ?
+ Paths.get(localPkgsDir) :
+ Paths.get(coreContainer.getSolrHome()).resolve(localPkgsDir);
log.info("Packages to be loaded from directory: {}", packagesPath);
- if (!packagesPath.toFile().exists()) {
+ if (!Files.exists(packagesPath)) {
Review comment:
Sys prop is the only way to do it now. We should NOT have an API to do this because this also goes against the `immutable deployments` narrative . However we may, in the future, use other startup mechanisms to configure the same (say `solr.xml`)
--
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