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 2019/12/26 01:40:06 UTC
[GitHub] [lucene-solr] noblepaul opened a new pull request #1124:
SOLR:14151: Schema components to be loadable from packages
noblepaul opened a new pull request #1124: SOLR:14151: Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124
DO NOT commit WIP
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361541789
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSet.java
##########
@@ -28,17 +30,17 @@
private final SolrConfig solrconfig;
- private final IndexSchema indexSchema;
+ private volatile Supplier<IndexSchema> indexSchemaSupplier;
Review comment:
Why volatile and no longer final? I only see one write to this in the constructor.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul edited a comment on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul edited a comment on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569546712
All tests are passing and I'm planning to merge this soon, if there are no other changes required
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request
#1124: SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361673142
##########
File path: solr/core/src/java/org/apache/solr/core/PluginLoader.java
##########
@@ -0,0 +1,30 @@
+/*
+ * 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 classes
Review comment:
π
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569546712
All tests are passing and I'm planning to merge this soon
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361778082
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAwareSolrClassLoader.java
##########
@@ -0,0 +1,154 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.BiFunction;
+
+import org.apache.solr.core.PluginInfo;
+import org.apache.solr.core.SolrClassLoader;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.core.SolrResourceLoader;
+import org.apache.solr.util.plugin.SolrCoreAware;
+
+import static java.util.Collections.singletonMap;
+
+/**
+ * This class implements a SolrClassLoader that can identify the correct packages
+ * and load classes from that. This also listens to any changes to the relevant packages and
+ * invoke a callback if anything is modified
+ */
+public class PackageAwareSolrClassLoader implements SolrClassLoader {
+ final SolrCore core;
+ final SolrResourceLoader loader;
+ private Map<String, PackageAPI.PkgVersion> classNameVsPkg = new HashMap<>();
+
+ private final List<PackageListeners.Listener> listeners = new ArrayList<>();
+ private final Runnable reloadRunnable;
+
+
+ public PackageAwareSolrClassLoader(SolrCore core, SolrResourceLoader loader, Runnable runnable) {
Review comment:
What's the runnable for?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361578602
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -935,6 +945,7 @@ public SolrCore(CoreContainer coreContainer, String name, String dataDir, SolrCo
MDCLoggingContext.setCore(this);
resourceLoader = config.getResourceLoader();
+ resourceLoader.core = this;
Review comment:
@janhoy see ConfigSetService.createCoreResourceLoader
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request
#1124: SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361456012
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchema.java
##########
@@ -1981,4 +2089,10 @@ public PayloadDecoder getPayloadDecoder(String field) {
return decoders.computeIfAbsent(ft, f -> PayloadUtils.getPayloadDecoder(ft));
}
+ @Override
+ public void close() throws IOException {
+ if (pluginLoader instanceof PackageAwarePluginLoader) {
Review comment:
defensive programming π
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361411570
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchema.java
##########
@@ -1981,4 +2089,10 @@ public PayloadDecoder getPayloadDecoder(String field) {
return decoders.computeIfAbsent(ft, f -> PayloadUtils.getPayloadDecoder(ft));
}
+ @Override
+ public void close() throws IOException {
+ if (pluginLoader instanceof PackageAwarePluginLoader) {
Review comment:
Why the instanceof check here?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request
#1124: SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361456012
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchema.java
##########
@@ -1981,4 +2089,10 @@ public PayloadDecoder getPayloadDecoder(String field) {
return decoders.computeIfAbsent(ft, f -> PayloadUtils.getPayloadDecoder(ft));
}
+ @Override
+ public void close() throws IOException {
+ if (pluginLoader instanceof PackageAwarePluginLoader) {
Review comment:
defensive , programming π
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request
#1124: SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361455888
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -935,6 +945,7 @@ public SolrCore(CoreContainer coreContainer, String name, String dataDir, SolrCo
MDCLoggingContext.setCore(this);
resourceLoader = config.getResourceLoader();
+ resourceLoader.core = this;
Review comment:
NO, SRL is per core
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request
#1124: SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361817112
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageAwareSolrClassLoader.java
##########
@@ -0,0 +1,154 @@
+/*
+ * 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 java.io.IOException;
+import java.io.InputStream;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.BiFunction;
+
+import org.apache.solr.core.PluginInfo;
+import org.apache.solr.core.SolrClassLoader;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.core.SolrResourceLoader;
+import org.apache.solr.util.plugin.SolrCoreAware;
+
+import static java.util.Collections.singletonMap;
+
+/**
+ * This class implements a SolrClassLoader that can identify the correct packages
+ * and load classes from that. This also listens to any changes to the relevant packages and
+ * invoke a callback if anything is modified
+ */
+public class PackageAwareSolrClassLoader implements SolrClassLoader {
+ final SolrCore core;
+ final SolrResourceLoader loader;
+ private Map<String, PackageAPI.PkgVersion> classNameVsPkg = new HashMap<>();
+
+ private final List<PackageListeners.Listener> listeners = new ArrayList<>();
+ private final Runnable reloadRunnable;
+
+
+ public PackageAwareSolrClassLoader(SolrCore core, SolrResourceLoader loader, Runnable runnable) {
Review comment:
It's to reload your set of plugins. In the case of schema it's a `refreshSchema()` , If you wish to reload core or something else , you could do it here
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569269915
> I see this introduces a new interface "PluginLoader" that is implemented by SolrResourceLoader and a new PackageAwarePluginLoader, and that it's only the latter that parses out the plugin information from a class name and not SolrResourceLoader (unlike the approach in my hackday). Avoiding SolrResourceLoader doing this is only feasible for Solr specific plugin loading code but not for code in Lucene that is based on a Lucene ResourceLoader. Just one example is SynonymGraphFilterFactory that can _itself_ load a Tokenizer and Analyzer via a provided ResourceLoader. Having SolrResourceLoader itself do plugin resolution will resolve this and doesn't introduce new abstractions (e.g. PluginLoader). WDYT?
I have renamed `PluginLoader` -> `SolrClassLoader` in the the latest commit
We need abstractions. Using concrete impls is one of the biggest reasons why we have very little modularity.
So, the solution to the problem is we will make `SolrClassLoader extends ResourceLoader` and this will be the interface which will be passed around. We will stop relying on SRL because SRL does too many things already. Piling on more things into an already bloated class is at best a hack. It's not a good idea to say that A package retsurns an SRL because most of the methods are invalid for such a use case
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361542052
##########
File path: solr/core/src/java/org/apache/solr/core/PluginInfo.java
##########
@@ -63,29 +63,42 @@ public PluginInfo(String type, Map<String, String> attrs, NamedList initArgs, Li
/** class names can be prefixed with package name e.g: my_package:my.pkg.Class
* This checks if it is a package name prefixed classname.
- * the return value has first = package name & second = class name
*/
- static Pair<String,String > parseClassName(String name) {
- String pkgName = null;
- String className = name;
- if (name != null) {
- int colonIdx = name.indexOf(':');
- if (colonIdx > -1) {
- pkgName = name.substring(0, colonIdx);
- className = name.substring(colonIdx + 1);
+
+ public static class ClassName {
Review comment:
I suggest "ParsedClass" consisting of "pkg", "clazz", and "original" (not "actual"). Though I'm not sure I like the very existence of the class; I'd rather it be private if it needs to exist.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r362097171
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSet.java
##########
@@ -51,8 +53,11 @@ public SolrConfig getSolrConfig() {
return solrconfig;
}
+ public Supplier<IndexSchema> getIndexSchemaSupplier() {
Review comment:
Nevermind Future; I was thinking FutureTask specifically but another look shows it'd block. However Guava `Suppliers.memoize` is perfect
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on issue #1124: SOLR-14151 :Schema
components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569389280
> So, the solution to the problem is we will make `SolrClassLoader extends ResourceLoader` and this will be the interface which will be passed around.
Sounds workable.
> We will stop relying on SRL because SRL does too many things already. Piling on more things into an already bloated class is at best a hack. It's not a good idea to say that A package returns an SRL because most of the methods are invalid for such a use case
I'm curious; what is some of the bloat of SRL that you think it should not do?
> The behavior of `SolrCoreAware` and `ResourceLoaderAware` is fixed in the latest commit
I'm glad to see that!
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request
#1124: SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361661140
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSet.java
##########
@@ -51,8 +53,11 @@ public SolrConfig getSolrConfig() {
return solrconfig;
}
+ public Supplier<IndexSchema> getIndexSchemaSupplier() {
Review comment:
Yeah, that is a problem. Can you tell me how the `Future` would look?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361408293
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -935,6 +945,7 @@ public SolrCore(CoreContainer coreContainer, String name, String dataDir, SolrCo
MDCLoggingContext.setCore(this);
resourceLoader = config.getResourceLoader();
+ resourceLoader.core = this;
Review comment:
Is this safe? Isnβt SRL shared by many cores?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361543380
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSet.java
##########
@@ -51,8 +53,11 @@ public SolrConfig getSolrConfig() {
return solrconfig;
}
+ public Supplier<IndexSchema> getIndexSchemaSupplier() {
Review comment:
I figure you have this in support of hot-updating? Please state as such in a comment.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361579553
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageListeners.java
##########
@@ -75,15 +78,32 @@ synchronized void packagesUpdated(List<PackageLoader.Package> pkgs) {
}
private synchronized void invokeListeners(PackageLoader.Package pkg) {
- for (Reference<Listener> ref : listeners) {
- Listener listener = ref.get();
- if(listener == null) continue;
- if (listener.packageName() == null || listener.packageName().equals(pkg.name())) {
- listener.changed(pkg);
+ Ctx ctx = new Ctx();
+
+ try {
+ for (Reference<Listener> ref : listeners) {
+ Listener listener = ref.get();
+ if(listener == null) continue;
+ if (listener.packageName() == null || listener.packageName().equals(pkg.name())) {
+ listener.changed(pkg, ctx);
+ }
+ }
+ } finally {
+ if(ctx.postProcessors != null){
+ for (Runnable value : ctx.postProcessors.values()) {
+ value.run();
+ }
}
}
}
+ public void forEachListener(Consumer<Listener> listenerConsumer){
+ listeners.forEach(ref -> {
Review comment:
Consider this instead:
```listeners.stream().map(Reference::get).filter(Objects::nonNull).forEach(listenerConsumer);```
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul edited a comment on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul edited a comment on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569603722
I would like to take a stab at the overall design
## Various classes
- `interface ResourceLoader` : The lucene interface that loads classes and resources
- `interface SolrClassLoader extends ResourceLaoder` : Has some extra overloaded methods to load plugins with a few extra paramaters
- `class SolrResourceLoader implements SolrClassLoader` : implements loading classes from a set of files on disk and it also has some extra methods pertaining to Solr (data dir, conf dir etc, etc). These things do not work with packages b/c packages do not support concepts like data dir, schema config etc.
- `class PackageAwareSolrClassLoader implements SolrClassLoader`: Has extra functionality of listening to package changes and give callback to reload something if anything changes. This outsources the classloading from file system to `SolrResourceLoader` itself.
## Hotloading
There are 2 types of hot loading implemented as of today
1. In place update of individual plugins: Most components configured in `solrconfig.xml` today support hotloading. Each object listens to its package changes and reload
2. schema : This treats the entire `IndexSchema` object as a single entity. Each object listens to package changes , if any package is modified, the entire `IndexSchema` is reloaded
3. TODO : We can support a new class of plugins which actually can reload the `SolrCore` when packages are updated. #1109 can use the `PackageAwareSolrClassLoader` to listen to and reload core
Your worst fears can be addressed with the following
- Is there any component in IndexSchema that is referred and stored outside of IndexSchema object?
- The following method in `SolrCore` must be broken , if this is true.
```
public void setLatestSchema(IndexSchema replacementSchema)
```
AFAIK, there are no known issues
In the worst case we can provide an extra hook for you to reload core always when schema components are changed
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul edited a comment on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul edited a comment on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569603722
I would like to take a stab at the overall design
## Various classes
- `interface ResourceLoader` : The lucene interface that loads classes and resources
- `interface SolrClassLoader extends ResourceLaoder` : Has some extra overloaded methods to load plugins with a few extra paramaters
- `class SolrResourceLoader implements SolrClassLoader` : implements loading classes from a set of files on disk and it also has some extra methods pertaining to Solr (data dir, conf dir etc, etc). These things do not work with packages b/c packages do not support concepts like data dir, schema config etc.
- `class PackageAwareSolrClassLoader implements SolrClassLoader`: Has extra functionality of listening to package changes and give callback to reload something if anything changes. This outsources the classloading from file system to `SolrResourceLoader` itself.
## Hotloading
There are 2 types of hot loading implemented as of today
1. In place update of individual plugins: Most components configured in `solrconfig.xml` today support hotloading. Each object listens to its package changes and reload
2. schema : This treats the entire `IndexSchema` object as a single entity. Each object listens to package changes , if any package is modified, the entire `IndexSchema` is reloaded
3. TODO : We can support a new class of plugins which actually can reload the `SolrCore` when packages are updated. #1109 can use the `PackageAwareSolrClassLoader` to listen packages to and reload core
Your worst fears can be addressed with the following
- Is there any component in IndexSchema that is referred and stored outside of IndexSchema object?
- The following method in `SolrCore` must be broken , if this is true.
```
public void setLatestSchema(IndexSchema replacementSchema)
```
AFAIK, there are no known issues
In the worst case we can provide an extra hook for you to reload core always when schema components are changed
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361777990
##########
File path: solr/core/src/java/org/apache/solr/handler/SchemaHandler.java
##########
@@ -206,6 +206,10 @@ 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
Review comment:
minor: nor formatted well (and same goes for many other added javadocs)
I love the new comment though
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361777912
##########
File path: solr/core/src/java/org/apache/solr/core/MemClassLoader.java
##########
@@ -53,6 +53,16 @@ public MemClassLoader(List<PluginBag.RuntimeLib> libs, SolrResourceLoader resour
this.libs = libs;
}
+ @Override
+ public <T> T newInstance(String cname, Class<T> expectedType, String... subpackages) {
+ return null;
Review comment:
I suggest adding "nocommit" strings to remind us there is work to do
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on issue #1124: SOLR-14151 :Schema
components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569594631
I appreciate that your perspective is that I am engaging in "bike shedding" -- debating matters of little substance. From my perspective, I am concerned with complexity, and thus the _how_ matters. I've realized a long time ago that writing code that works (notwithstanding unforeseen bugs) is only part of what matters and doesn't count for much. What also matters is lots of quality "ilities" -- complexity (/ simplicity), extensibility, readability, efficiency, and various others. All are somewhat subjective characteristics that differentiate one working solution from another. The solutions developed by the 'n' developers in your example are not equivalent just because they all work. These quality characteristics impact the future of the project and all of us maintaining it going forward.
I fear the complexity impact in this PR. A new abstraction that will be used widely that competes with an existing one isn't my biggest concern but it's one I thought I could articulate well (though it seems I'm doing poorly). My biggest complexity concern is the impact of supporting hot-loading. Hot-loading is a cool feature to tell users about but the complexity implications of the implementation scare me, and more so here at the schema. It's not just some feature that stays on the sidelines if not used (e.g. some misc query parser); this affects our foundation. By "scare me"; it's like what you were saying earlier about code we are scared to touch because we don't understand it (due to complexity of course).
I confess; some of my fear is mere ignorance of what I know little about. You could help with that. Perhaps help explain how schema hot reloading works. And then ultimately put that in javadoc somewhere. Frankly this patch is getting too complex for me to see for myself; I've never actually gotten through the whole thing. I can't see the forest for the trees.
-0 to commit as is. I can understand you want to commit any way; you seem abundantly confident in everything you've done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul edited a comment on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul edited a comment on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569269915
> I see this introduces a new interface "PluginLoader" that is implemented by SolrResourceLoader and a new PackageAwarePluginLoader, and that it's only the latter that parses out the plugin information from a class name and not SolrResourceLoader (unlike the approach in my hackday). Avoiding SolrResourceLoader doing this is only feasible for Solr specific plugin loading code but not for code in Lucene that is based on a Lucene ResourceLoader. Just one example is SynonymGraphFilterFactory that can _itself_ load a Tokenizer and Analyzer via a provided ResourceLoader. Having SolrResourceLoader itself do plugin resolution will resolve this and doesn't introduce new abstractions (e.g. PluginLoader). WDYT?
I have renamed `PluginLoader` -> `SolrClassLoader` in the the latest commit
We need abstractions. Using concrete impls is one of the biggest reasons why we have very little modularity.
So, the solution to the problem is we will make `SolrClassLoader extends ResourceLoader` and this will be the interface which will be passed around. We will stop relying on SRL because SRL does too many things already. Piling on more things into an already bloated class is at best a hack. It's not a good idea to say that A package returns an SRL because most of the methods are invalid for such a use case
The current behavior is not correct. We should invoke `inform(ResourceLoader)` soon after the objects are constructed b/c a package Loader is ready .
The behavior of `SolrCoreAware` and `ResourceLoaderAware` is fixed in the latest commit
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul edited a comment on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul edited a comment on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569269915
> I see this introduces a new interface "PluginLoader" that is implemented by SolrResourceLoader and a new PackageAwarePluginLoader, and that it's only the latter that parses out the plugin information from a class name and not SolrResourceLoader (unlike the approach in my hackday). Avoiding SolrResourceLoader doing this is only feasible for Solr specific plugin loading code but not for code in Lucene that is based on a Lucene ResourceLoader. Just one example is SynonymGraphFilterFactory that can _itself_ load a Tokenizer and Analyzer via a provided ResourceLoader. Having SolrResourceLoader itself do plugin resolution will resolve this and doesn't introduce new abstractions (e.g. PluginLoader). WDYT?
I have renamed `PluginLoader` -> `SolrClassLoader` in the the latest commit
We need abstractions. Using concrete impls is one of the biggest reasons why we have very little modularity.
So, the solution to the problem is we will make `SolrClassLoader extends ResourceLoader` and this will be the interface which will be passed around. We will stop relying on SRL because SRL does too many things already. Piling on more things into an already bloated class is at best a hack. It's not a good idea to say that A package retsurns an SRL because most of the methods are invalid for such a use case
The current behavior is not correct. We should invoke `inform(ResourceLoader)` soon after the objects are constructed b/c a package Loader is ready .
The behavior of `SolrCoreAware` and `ResourceLoaderAware` is fixed in the latest commit
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361408428
##########
File path: solr/core/src/java/org/apache/solr/core/PluginInfo.java
##########
@@ -63,29 +63,42 @@ public PluginInfo(String type, Map<String, String> attrs, NamedList initArgs, Li
/** class names can be prefixed with package name e.g: my_package:my.pkg.Class
* This checks if it is a package name prefixed classname.
- * the return value has first = package name & second = class name
*/
- static Pair<String,String > parseClassName(String name) {
- String pkgName = null;
- String className = name;
- if (name != null) {
- int colonIdx = name.indexOf(':');
- if (colonIdx > -1) {
- pkgName = name.substring(0, colonIdx);
- className = name.substring(colonIdx + 1);
+
+ public static class ClassName {
Review comment:
Find a better name for this class?
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul edited a comment on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul edited a comment on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569546583
>It's as if you can't bear to say you like SRL so you create something else in its image but without the dead weight we agree shouldn't be in SRL.
SRL is fine today as a class. Did I create something else in its image? NO. I just applied some lipstick on to that cute little piglet. It's just an interface. I don't even know why we are discussing SRL here. It's unproductive and we are not solving any problems with that.
Now, we have a solution that works. This already covers 90% of what most users would need. We will have more productive debate if we start discussing
- Is there a problem with the PR? Is it bad code? Is it working wrong? Is it inefficient?
- How do we cover the last mile?
If 'n' developers are asked to solve a problem, we will get 'n' solutions. We can keep on bikeshedding why solution A is not as good as solution B or vice versa. We just solve it and move on. Solr has a lot of problems to solve. We are just worried about the color of the bike shed
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569546583
>It's as if you can't bear to say you like SRL so you create something else in its image but without the dead weight we agree shouldn't be in SRL.
SRL is fine today as a class. Did I create something in its image? NO. I just applied some lipstick on to that cute little piglet. It's just an interface. I don't even know why we are discussing SRL here. It's unproductive and we are not solving any problems with that.
Now, we have a solution that works. This already covers 90% of what most users would need. We will have more productive debate if we start discussing
- Is there a problem with the PR? Is it bad code? Is it working wrong? Is it inefficient?
- How do we cover the last mile?
If 'n' developers are asked to solve a problem, we will get 'n' solutions. We can keep on bikeshedding why solution A is not as good as solution B or vice versa. We just solve it and move on. Solr has a lot of problems to solve. We are just worried about the color of the bike shed
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on issue #1124: SOLR-14151 :Schema
components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569474142
> The problem is that SRL is directly used in all APIs
I don't consider this a problem. Solr has a lot of plugin abstractions and resources that need loading and at least there is a single abstraction (SRL) that serves this purpose. If there were a bunch of loading abstractions then _that_ may be a problem (IMO), but that is not the case yet thankfully. It may be becoming the case here so I'm paying attention to complexity.
> ```
> public String getConfigDir()
>
> public String getDataDir()
>
> public Properties getCoreProperties()
>
> public InputStream openConfig(String name) throws IOException
>
> public InputStream openSchema(String name)
> ```
I don't like those methods being there either! Yay; we agree. There are some more that seem dubious to me too, e.g. `persistConfLocally`; _blech!_
> So the common sense thing to do is to create an interface and hide these methods
I think that's one option (the one you are taking). I don't care to say what is or isn't "common sense". A down side to this approach is that the system is now more complex as a whole; the SRL you don't like is still there. I'm curious; do you think these unsightly methods on SRL (that we agree should go away) are really fundamental problems with SRL and get in the way of what we are doing today?
> Slapping package loading responsibilities on to SRL is the most dangerous thing to do. We have no separation of responsibilities, and we have a class we are all afraid to touch because it may break something and we nobody knows how it works. Sol already has enough such pieces
You are a man of strong opinions. I very much disagree with you concerning your view on SRL. SRL has been loading plugins since before a package management system came along. If it worked with a package manager, that seems to me the same responsibility -- it still loads plugins. The implementation would then change to also work with a package manager but it would still be responsible for loading plugins.
Perhaps we actually agree much more than it appears. You are proposing a new abstraction that looks _a lot_ like SRL: SolrClassLoader. It's as if you can't bear to say you like SRL so you create something else in its image but without the dead weight we agree shouldn't be in SRL. Can't we instead work to "fix" SRL and have one abstraction?
I very much share your concern of being wary of code that "nobody knows how it works" but I don't view SRL as such. You can count me as someone who can help you or anyone understand SRL should you need help ;-). Now _IndexFetcher_ there's a beast I'm scared of.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] janhoy commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361755928
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchema.java
##########
@@ -1981,4 +2089,10 @@ public PayloadDecoder getPayloadDecoder(String field) {
return decoders.computeIfAbsent(ft, f -> PayloadUtils.getPayloadDecoder(ft));
}
+ @Override
+ public void close() throws IOException {
+ if (pluginLoader instanceof PackageAwarePluginLoader) {
Review comment:
Ok, I see - we create this ourselves and then we have to close it :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361543861
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSet.java
##########
@@ -51,8 +53,11 @@ public SolrConfig getSolrConfig() {
return solrconfig;
}
+ public Supplier<IndexSchema> getIndexSchemaSupplier() {
Review comment:
Based on what I'm seeing in this PR, it appears this Supplier will in fact load the schema each time get() is called? If so that sounds easy to accidentally over-load the schema. Might a `Future` work 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569603722
I would like to take a stab at what is the overall design
## Various classes
- `interface ResourceLoader` : The lucene interface that loads classes and resources
- `interface SolrClassLoader extends ResourceLaoder` : Has some extra overloaded methods to load plugins with a few extra paramaters
- `class SolrResourceLoader implements SolrClassLoader` : implements loading classes from a set of files on disk and it also has some extra methods pertaining to Solr (data dir, conf dir etc, etc). These things do not work with packages b/c packages do not support concepts like data dir, schema config etc.
- `class PackageAwareSolrClassLoader implements SolrClassLoader`: Has extra functionality of listening to package changes and give callback to reload something if anything changes. This outsources the classloading from file system to `SolrResourceLoader` itself.
## Hotloading
There are 2 types of hot loading implemented as of today
1. In place update of individual plugins: Most components configured in `solrconfig.xml` today support hotloading. Each object listens to its package changes and reload
2. schema : This treats the entire `IndexSchema` object as a single entity. Each object listens to package changes , if any package is modified, the entire `IndexSchema` is reloaded
3. TODO : We can support a new class of plugins which actually can reload the `SolrCore` when packages are updated. #1109 can use the `PackageAwareSolrClassLoader` to listen to and reload core
Your worst fears can be addressed with the following
- Is there any component in IndexSchema that is referred and stored outside of IndexSchema object?
- The following method in `SolrCore` must be broken , if this is true.
```
public void setLatestSchema(IndexSchema replacementSchema)
```
AFAIK, there are no known issues
In the worst case we can provide an extra hook for you to reload core always when schema components are changed
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request
#1124: SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361817070
##########
File path: solr/core/src/java/org/apache/solr/core/MemClassLoader.java
##########
@@ -53,6 +53,16 @@ public MemClassLoader(List<PluginBag.RuntimeLib> libs, SolrResourceLoader resour
this.libs = libs;
}
+ @Override
+ public <T> T newInstance(String cname, Class<T> expectedType, String... subpackages) {
+ return null;
Review comment:
Actually , we are getting rid of `MemClassLoader`. It's a part of the `runtimeLib` feature. So, we don't need to implement any of these
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361578750
##########
File path: solr/core/src/java/org/apache/solr/handler/SchemaHandler.java
##########
@@ -200,6 +206,38 @@ private void handleGET(SolrQueryRequest req, SolrQueryResponse rsp) {
}
}
+ private void insertPackageInfo(Object o, SolrQueryRequest req) {
Review comment:
Could use a javadoc line saying what is put where. Also if it took SolrQueryResponse and not 'o', I think it would be clearer.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul edited a comment on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul edited a comment on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569269915
> I see this introduces a new interface "PluginLoader" that is implemented by SolrResourceLoader and a new PackageAwarePluginLoader, and that it's only the latter that parses out the plugin information from a class name and not SolrResourceLoader (unlike the approach in my hackday). Avoiding SolrResourceLoader doing this is only feasible for Solr specific plugin loading code but not for code in Lucene that is based on a Lucene ResourceLoader. Just one example is SynonymGraphFilterFactory that can _itself_ load a Tokenizer and Analyzer via a provided ResourceLoader. Having SolrResourceLoader itself do plugin resolution will resolve this and doesn't introduce new abstractions (e.g. PluginLoader). WDYT?
I have renamed `PluginLoader` -> `SolrClassLoader` in the the latest commit
We need abstractions. Using concrete impls is one of the biggest reasons why we have very little modularity.
So, the solution to the problem is we will make `SolrClassLoader extends ResourceLoader` and this will be the interface which will be passed around. We will stop relying on SRL because SRL does too many things already. Piling on more things into an already bloated class is at best a hack. It's not a good idea to say that A package retsurns an SRL because most of the methods are invalid for such a use case
The current behavior is not correct. We should invoke `inform(ResourceLoader)` soon after the objects are constructed b/c a package Loader is ready even before
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1124:
SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361542449
##########
File path: solr/core/src/java/org/apache/solr/core/PluginLoader.java
##########
@@ -0,0 +1,30 @@
+/*
+ * 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 classes
Review comment:
Can you please either (A) put the first javadoc sentence on the following line and not adjacent to the asterisk *or* (B) have the entire javadoc syntax be the one line since it's very short.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request
#1124: SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361455949
##########
File path: solr/core/src/java/org/apache/solr/core/PluginInfo.java
##########
@@ -63,29 +63,42 @@ public PluginInfo(String type, Map<String, String> attrs, NamedList initArgs, Li
/** class names can be prefixed with package name e.g: my_package:my.pkg.Class
* This checks if it is a package name prefixed classname.
- * the return value has first = package name & second = class name
*/
- static Pair<String,String > parseClassName(String name) {
- String pkgName = null;
- String className = name;
- if (name != null) {
- int colonIdx = name.indexOf(':');
- if (colonIdx > -1) {
- pkgName = name.substring(0, colonIdx);
- className = name.substring(colonIdx + 1);
+
+ public static class ClassName {
Review comment:
I didn't like the name either. Suggestions welcome
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul edited a comment on issue #1124: SOLR-14151
:Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul edited a comment on issue #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#issuecomment-569546583
>It's as if you can't bear to say you like SRL so you create something else in its image but without the dead weight we agree shouldn't be in SRL.
> the SRL you don't like is still there.
SRL is fine today as a class & I don't hate it at all. Did I create something else in its image? NO. I just applied some lipstick on to that cute little piglet. It's just an interface. I don't even know why we are discussing SRL here. It's unproductive and we are not solving any problems with that.
> A down side to this approach is that the system is now more complex as a whole;
Yes, the system is more complex because it is expected to do new things. We just hide the complexity behind abstractions. People do not have to deal with the complexity unless they need to. They should be given clear interfaces/APIs to deal with it. If they ever need to know how it works, they just peel off the interface and look at the implementation.
Now, we have a solution that works. This already covers >90% of what most users would need. We will have more productive debate if we start discussing
- Is there a problem with the PR? Is it bad code? Is it working wrong? Is it inefficient?
- How do we cover the last mile?
If 'n' developers are asked to solve a problem, we will get 'n' solutions. We can keep on bikeshedding why solution A is not as good as solution B or vice versa. We just solve it and move on. Solr has a lot of problems to solve. We are just worried about the color of the bike shed
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request
#1124: SOLR-14151 :Schema components to be loadable from packages
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1124: SOLR-14151 :Schema components to be loadable from packages
URL: https://github.com/apache/lucene-solr/pull/1124#discussion_r361661294
##########
File path: solr/core/src/java/org/apache/solr/core/ConfigSet.java
##########
@@ -28,17 +30,17 @@
private final SolrConfig solrconfig;
- private final IndexSchema indexSchema;
+ private volatile Supplier<IndexSchema> indexSchemaSupplier;
Review comment:
It was a mistake , fixed
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org