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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
