You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/07/14 15:27:00 UTC
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1669: SOLR-14151 Make schema components load from packages
dsmiley commented on a change in pull request #1669:
URL: https://github.com/apache/lucene-solr/pull/1669#discussion_r454404343
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -198,6 +206,11 @@ synchronized void reloadLuceneSPI() {
TokenizerFactory.reloadTokenizers(this.classLoader);
}
+ public SolrCore getCore(){
Review comment:
This is not used?
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageListeners.java
##########
@@ -96,15 +97,42 @@ private synchronized void invokeListeners(PackageLoader.Package pkg) {
public interface Listener {
- /**Name of the package or null to loisten to all package changes
+ /**Name of the package or null to listen to all package changes
*/
String packageName();
PluginInfo pluginInfo();
- void changed(PackageLoader.Package pkg);
+ void changed(PackageLoader.Package pkg, Ctx ctx);
PackageLoader.Package.Version getPackageVersion();
+ class Ctx {
+ private Map<String, Runnable > runLater;
+
+ /** If there are multiple packages to be updated and there are multiple listeners,
+ * This is executed after all of the {@link Listener#changed(PackageLoader.Package, Ctx)}
+ * calls are invoked. The name is a unique identifier that can be used by consumers to avoid duplicate
+ * If no deduplication is required, use null
+ * runnable objects
+ */
+ public void runLater(String name, Runnable runnable ) {
+ if(runLater == null) runLater = new LinkedHashMap<>();
+ if(name == null) {
+ name = runnable.getClass().getSimpleName() + "@" + runnable.hashCode();
+ }
+ runLater.put(name, runnable);
+ }
+ private void runLaterTasks(){
+ if(runLater == null) return;
+ new Thread(() -> runLater.forEach((s, runnable) -> {
Review comment:
Could use find a Solr based ExecutorService for this instead? It sets up MDC and we ensure it gets shut down.
##########
File path: solr/core/src/java/org/apache/solr/handler/SchemaHandler.java
##########
@@ -202,6 +202,38 @@ private void handleGET(SolrQueryRequest req, SolrQueryResponse rsp) {
}
}
+ /**If a plugin is loaded from a package, the version of the package being used should be added
+ * to the response
+ *
+ */
+ @SuppressWarnings("rawtypes")
+ private void insertPackageInfo(Object o, SolrQueryRequest req) {
+ if(!req.getParams().getBool("meta",false)) return;
Review comment:
nitpick: auto-format this code to be consistent with spaces
##########
File path: solr/core/src/java/org/apache/solr/handler/StreamHandler.java
##########
@@ -158,8 +158,8 @@ public Class getClazz() {
}
@Override
- protected void initNewInstance(PackageLoader.Package.Version newest) {
- clazz = newest.getLoader().findClass(pluginInfo.className, Expressible.class);
+ protected Object initNewInstance(PackageLoader.Package.Version newest) {
+ return clazz = newest.getLoader().findClass(pluginInfo.className, Expressible.class);
Review comment:
This code here returns a class and not an instance. This seems wrong?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -752,6 +765,9 @@ public void close() throws IOException {
}
+ public CoreContainer getCoreContainer(){
Review comment:
this is not used?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrClassLoader.java
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.
+ */
+package org.apache.solr.core;
+
+
+/**A generic interface to load plugin classes
Review comment:
nit: see my longer comment on formatting javadoc. This case right here really gets to me.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageListeners.java
##########
@@ -96,15 +97,42 @@ private synchronized void invokeListeners(PackageLoader.Package pkg) {
public interface Listener {
- /**Name of the package or null to loisten to all package changes
+ /**Name of the package or null to listen to all package changes
*/
String packageName();
PluginInfo pluginInfo();
- void changed(PackageLoader.Package pkg);
+ void changed(PackageLoader.Package pkg, Ctx ctx);
PackageLoader.Package.Version getPackageVersion();
+ class Ctx {
+ private Map<String, Runnable > runLater;
+
+ /** If there are multiple packages to be updated and there are multiple listeners,
+ * This is executed after all of the {@link Listener#changed(PackageLoader.Package, Ctx)}
+ * calls are invoked. The name is a unique identifier that can be used by consumers to avoid duplicate
+ * If no deduplication is required, use null
+ * runnable objects
+ */
+ public void runLater(String name, Runnable runnable ) {
+ if(runLater == null) runLater = new LinkedHashMap<>();
Review comment:
nit: please auto-format for space consistency
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageListeners.java
##########
@@ -96,15 +97,42 @@ private synchronized void invokeListeners(PackageLoader.Package pkg) {
public interface Listener {
- /**Name of the package or null to loisten to all package changes
+ /**Name of the package or null to listen to all package changes
Review comment:
nit:
Single-line javadocs can entirely be in one line:
/** Name of the package or null to listen to all package changes */
Multi-line are formatted like this:
/**
* Summary sentence.
* More info.
*/
I see this formatting inconsistency in lots of your javadocs. I know it's not a big deal yet it's still gives code not adhering to this a sloppy feel. I find https://google.github.io/styleguide/javaguide.html#s7-javadoc useful to refer to, and it's perhaps the most popular Java style guide.
You might try Opt-Cmd-L if you use IntelliJ on a Mac to reformat selected text.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageListeners.java
##########
@@ -96,15 +97,42 @@ private synchronized void invokeListeners(PackageLoader.Package pkg) {
public interface Listener {
- /**Name of the package or null to loisten to all package changes
+ /**Name of the package or null to listen to all package changes
*/
String packageName();
PluginInfo pluginInfo();
- void changed(PackageLoader.Package pkg);
+ void changed(PackageLoader.Package pkg, Ctx ctx);
PackageLoader.Package.Version getPackageVersion();
+ class Ctx {
+ private Map<String, Runnable > runLater;
+
+ /** If there are multiple packages to be updated and there are multiple listeners,
+ * This is executed after all of the {@link Listener#changed(PackageLoader.Package, Ctx)}
+ * calls are invoked. The name is a unique identifier that can be used by consumers to avoid duplicate
+ * If no deduplication is required, use null
+ * runnable objects
Review comment:
I think you mean, use null _for the name_ and not for the Runnable object. The runnable is required.
##########
File path: solr/core/src/java/org/apache/solr/pkg/SchemaPluginsLoader.java
##########
@@ -0,0 +1,160 @@
+/*
+ * 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.
+ */
+
+package org.apache.solr.pkg;
+
+import org.apache.lucene.analysis.util.ResourceLoaderAware;
+import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
+import org.apache.solr.core.CoreContainer;
+import org.apache.solr.core.PluginInfo;
+import org.apache.solr.core.SolrClassLoader;
+import org.apache.solr.core.SolrResourceLoader;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.Function;
+/**
+ * A {@link SolrClassLoader} that is specifically designed to load schema plugins from packages.
+ * This class would register a listener for any package that is used in a schema and reload the schema
+ * if any of those packages are updated
+ * */
+public class SchemaPluginsLoader implements SolrClassLoader {
+ final CoreContainer coreContainer;
+ final SolrResourceLoader loader;
+ final Function<String, String> pkgVersionSupplier;
+ private Map<String ,PackageAPI.PkgVersion> packageVersions = new HashMap<>(1);
Review comment:
nit: spacing issue.
Also, please use 'final' for all fields that are final and not just some. Likewise for 'private' for fields than can be private.
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchema.java
##########
@@ -188,6 +190,7 @@ public IndexSchema(String name, InputSource is, Version luceneVersion, SolrResou
protected IndexSchema(Version luceneVersion, SolrResourceLoader loader, Properties substitutableProperties) {
this.luceneVersion = Objects.requireNonNull(luceneVersion);
this.loader = loader;
+ this.solrClassLoader = loader.getCore() == null? loader: loader.getCore().getSchemaPluginsLoader();
Review comment:
This seems problematic. The code involved in index schema loading has access to two fields that both implement SolrClassLoader: `loader` and `solrClassLoader` which you just added. And then you changed many lines to use SolrClassLoader which just as well could have been as it was before -- `loader` (SRL). I can see that you're doing this so that a new SchemaPluginsLoader thing could be used. What if SchemaPluginsLoader was an SRL itself, and delegated the resource-loading methods to the "real" SRL?
Put differently, we could create a synthetic SRL whose methods delegate to an appropriate plugin enabled ClassLoader and a real SRL for the configSet to find resources. WDYT?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -191,6 +191,11 @@
private String name;
private String logid; // used to show what name is set
+ /**
+ * A unique id to differentiate multiple instances of the same core
+ * If we reload a core, the name remains same , but the id will be new
+ */
+ public final UUID uniqueId = UUID.randomUUID();
Review comment:
I don't think we need UUID because AFAICT the usage is entirely within the node. Instead, just use a static AtomicLong counter like, for example, SearchHandler.ridCounter and then declare this instance field here as a simple primitive long.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAPI.java
##########
@@ -52,6 +42,11 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.IOException;
Review comment:
nit: you re-ordered the imports in a way not consistent with most classes. java package should come first.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org