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-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
