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/05/31 09:54:54 UTC
[GitHub] [lucene-solr] noblepaul opened a new pull request #1547: SOLR-14525 For components loaded from packages SolrCoreAware, ResourceLoaderAware are not honored
noblepaul opened a new pull request #1547:
URL: https://github.com/apache/lucene-solr/pull/1547
SOLR-14525
For components loaded from packages SolrCoreAware, ResourceLoaderAware are not honored
----------------------------------------------------------------
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
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1547: SOLR-14525 For components loaded from packages SolrCoreAware, ResourceLoaderAware are not honored
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1547:
URL: https://github.com/apache/lucene-solr/pull/1547#discussion_r433026497
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -17,31 +17,25 @@
package org.apache.solr.pkg;
-import java.io.Closeable;
-import java.io.IOException;
-import java.lang.invoke.MethodHandles;
-import java.nio.file.Path;
-import java.nio.file.Paths;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.CopyOnWriteArrayList;
-
+import org.apache.lucene.analysis.util.ResourceLoaderAware;
import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.core.CoreContainer;
import org.apache.solr.core.SolrCore;
import org.apache.solr.core.SolrResourceLoader;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.Closeable;
Review comment:
Can you please configure your IDE to put java.* import statements up front instead of below?
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -301,6 +295,32 @@ public String toString() {
}
}
}
+ static class PackageResourceLoader extends SolrResourceLoader {
+
+ PackageResourceLoader(String name, List<Path> classpath, Path instanceDir, ClassLoader parent) {
+ super(name, classpath, instanceDir, parent);
+ }
+
+ @Override
+ public <T> boolean addToCoreAware(T obj) {
+ //do not do anything
+ //this class is
Review comment:
incomplete comment? Could use an explanation as to why. SolrCoreAware isn't handled here.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackagePluginHolder.java
##########
@@ -117,6 +114,13 @@ protected void initNewInstance(PackageLoader.Package.Version newest) {
Object instance = SolrCore.createInstance(pluginInfo.className,
pluginMeta.clazz, pluginMeta.getCleanTag(), core, newest.getLoader());
PluginBag.initInstance(instance, pluginInfo);
+ if (instance instanceof SolrCoreAware) {
Review comment:
Aha; makes sense.
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -301,6 +295,32 @@ public String toString() {
}
}
}
+ static class PackageResourceLoader extends SolrResourceLoader {
+
+ PackageResourceLoader(String name, List<Path> classpath, Path instanceDir, ClassLoader parent) {
+ super(name, classpath, instanceDir, parent);
+ }
+
+ @Override
+ public <T> boolean addToCoreAware(T obj) {
+ //do not do anything
+ //this class is
+ return false;
+ }
+
+ @Override
+ public <T> boolean addToResourceLoaderAware(T obj) {
+ if (obj instanceof ResourceLoaderAware) {
+ assertAwareCompatibility(ResourceLoaderAware.class, obj);
+ try {
+ ((ResourceLoaderAware) obj).inform(this);
Review comment:
Lets figure out why the base class doesn't just immediately call inform like this here. What test then fails? I've read an interesting JIRA issue: https://issues.apache.org/jira/browse/SOLR-8311 (lots of info there; I recommend reading all of it) It's been on my mind for some time now actually since when I was cleaning up SolrResourceLoader since this "live" wart seems like a hack.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -566,25 +566,42 @@ static void clearCache() {
} catch (Exception e) {
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
- "Error instantiating class: '" + clazz.getName() + "'", e);
+ "Error instantiating class: '" + clazz.getName() + "'", e);
}
- if (!live) {
- if (obj instanceof SolrCoreAware) {
- assertAwareCompatibility(SolrCoreAware.class, obj);
- waitingForCore.add((SolrCoreAware) obj);
+ addToCoreAware(obj);
+ addToResourceLoaderAware(obj);
+ if(!live) {
Review comment:
Should this not be in its own method now too?
----------------------------------------------------------------
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
[GitHub] [lucene-solr] noblepaul merged pull request #1547: SOLR-14525 For components loaded from packages SolrCoreAware, ResourceLoaderAware are not honored
Posted by GitBox <gi...@apache.org>.
noblepaul merged pull request #1547:
URL: https://github.com/apache/lucene-solr/pull/1547
----------------------------------------------------------------
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
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1547: SOLR-14525 For components loaded from packages SolrCoreAware, ResourceLoaderAware are not honored
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1547:
URL: https://github.com/apache/lucene-solr/pull/1547#discussion_r433146984
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -17,31 +17,25 @@
package org.apache.solr.pkg;
-import java.io.Closeable;
-import java.io.IOException;
-import java.lang.invoke.MethodHandles;
-import java.nio.file.Path;
-import java.nio.file.Paths;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.Objects;
-import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.CopyOnWriteArrayList;
-
+import org.apache.lucene.analysis.util.ResourceLoaderAware;
import org.apache.solr.common.MapWriter;
+import org.apache.solr.common.SolrException;
import org.apache.solr.common.cloud.ZkStateReader;
import org.apache.solr.core.CoreContainer;
import org.apache.solr.core.SolrCore;
import org.apache.solr.core.SolrResourceLoader;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.io.Closeable;
Review comment:
OK
##########
File path: solr/core/src/java/org/apache/solr/pkg/PackageLoader.java
##########
@@ -301,6 +295,32 @@ public String toString() {
}
}
}
+ static class PackageResourceLoader extends SolrResourceLoader {
+
+ PackageResourceLoader(String name, List<Path> classpath, Path instanceDir, ClassLoader parent) {
+ super(name, classpath, instanceDir, parent);
+ }
+
+ @Override
+ public <T> boolean addToCoreAware(T obj) {
+ //do not do anything
+ //this class is
+ return false;
+ }
+
+ @Override
+ public <T> boolean addToResourceLoaderAware(T obj) {
+ if (obj instanceof ResourceLoaderAware) {
+ assertAwareCompatibility(ResourceLoaderAware.class, obj);
+ try {
+ ((ResourceLoaderAware) obj).inform(this);
Review comment:
I think the order has to be
1. create instance
2. `init()`
3. `SolrCoreAware.inform()` , `ResourceLoaderAware.inform()`
I've made the relevant changes. Pls review
----------------------------------------------------------------
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