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/20 22:34:29 UTC
[GitHub] [lucene-solr] dsmiley opened a new pull request #1109: More
pervasive use of PackageLoader / PluginInfo
dsmiley opened a new pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109
WIP don't merge.
This is what I did during my "hack day". I'd be happy to discuss.
CC @noblepaul @chatman
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361021966
##########
File path: solr/core/src/java/org/apache/solr/core/backup/repository/BackupRepositoryFactory.java
##########
@@ -71,7 +71,7 @@ public BackupRepository newInstance(SolrResourceLoader loader, String name) {
PluginInfo repo = Objects.requireNonNull(backupRepoPluginByName.get(name),
"Could not find a backup repository with name " + name);
- BackupRepository result = loader.newInstance(repo.className, BackupRepository.class);
+ BackupRepository result = loader.newInstance(repo, BackupRepository.class);
Review comment:
no thought given to reload
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361061609
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -810,28 +809,30 @@ void initIndex(boolean passOnPreviousState, boolean reload) throws IOException {
* Creates an instance by trying a constructor that accepts a SolrCore before
* trying the default (no arg) constructor.
*
- * @param className the instance class to create
+ * @param pluginInfo the instance class to create
* @param cast the class or interface that the instance should extend or implement
- * @param msg a message helping compose the exception error if any occurs.
* @param core The SolrCore instance for which this object needs to be loaded
* @return the desired instance
* @throws SolrException if the object could not be instantiated
*/
- public static <T> T createInstance(String className, Class<T> cast, String msg, SolrCore core, ResourceLoader resourceLoader) {
- Class<? extends T> clazz = null;
- if (msg == null) msg = "SolrCore Object";
+ public static <T> T newInstance(PluginInfo pluginInfo, Class<T> cast, SolrCore core, SolrResourceLoader resourceLoader) {
+ String msg = pluginInfo.type;
try {
- clazz = resourceLoader.findClass(className, cast);
- //most of the classes do not have constructors which takes SolrCore argument. It is recommended to obtain SolrCore by implementing SolrCoreAware.
- // So invariably always it will cause a NoSuchMethodException. So iterate though the list of available constructors
- Constructor<?>[] cons = clazz.getConstructors();
- for (Constructor<?> con : cons) {
- Class<?>[] types = con.getParameterTypes();
- if (types.length == 1 && types[0] == SolrCore.class) {
- return cast.cast(con.newInstance(core));
+ //TODO separate out "core" scenario to another method
+ if (pluginInfo.pkgName == null && core != null) {
+ Class<? extends T> clazz = resourceLoader.findClass(pluginInfo.className, cast);
Review comment:
Quite right Noble! In so doing, with low effort we enable all plugins to be loaded at runtime, _even the schema plugins!_. "Hot update" is for another day here; one step at a time.
----------------------------------------------------------------
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 #1109: More pervasive use
of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on issue #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-569088078
I like the 3 categories of plugins very much, and also especially glad and surprised to hear you state "Hot reloading is not important.". Eventually we'll want to document this in some way by-plugin abstraction so users understand in the ref guide. And if you try to refer to a plugin when we know we can't support it, then we should tell the user more explicitly so.
I still need to look at your schema PR #1124
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361234213
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -954,4 +987,46 @@ public static void persistConfLocally(SolrResourceLoader loader, String resource
}
}
+ // TODO document these methods...
Review comment:
SRL has nothing to do with packages. It's an anti pattern. SRL is used by PackageLaoder and SRL is totally agnostic of the package system. We should just limit exposure of Packages to 2 places
- `CoreContainer` : This is where the package loader is stored
- `SolrCore` : This is where the correct version of a package for a plugin can be identified and listeners for package updates are registered
Without the correct version, we leave the system inconsistent
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361017640
##########
File path: solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java
##########
@@ -275,7 +276,7 @@ private VelocityContext createContext(SolrQueryRequest request, SolrQueryRespons
for (Map.Entry<String, String> entry : customTools.entrySet()) {
String name = entry.getKey();
// TODO: at least log a warning when one of the *fixed* tools classes is same name with a custom one, currently silently ignored
- Object customTool = SolrCore.createInstance(entry.getValue(), Object.class, "VrW custom tool: " + name, request.getCore(), request.getCore().getResourceLoader());
+ Object customTool = SolrCore.newInstance(new PluginInfo(name, entry.getValue()), Object.class, request.getCore(), request.getCore().getResourceLoader());
Review comment:
What is the purpose of this? Apparently this is not even using the right Classloader
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361278515
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##########
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className, IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info, IndexSchemaFactory.class);
Review comment:
That's plain wrong. SRL has no idea which version of package should be loaded
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361021282
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -890,7 +897,7 @@ private UpdateHandler createReloadedUpdateHandler(String className, String msg,
}
private UpdateHandler createUpdateHandler(String className) {
- return createInstance(className, UpdateHandler.class, "Update Handler", this, getResourceLoader());
+ return newInstance(new PluginInfo("updateHandler", className), UpdateHandler.class, this, getResourceLoader());
Review comment:
Again. No thought given to how it updates, if package is updated
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361021511
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -2905,7 +2912,7 @@ protected RestManager initRestManager() throws SolrException {
RestManager mgr = null;
if (restManagerPluginInfo != null) {
if (restManagerPluginInfo.className != null) {
- mgr = resourceLoader.newInstance(restManagerPluginInfo.className, RestManager.class);
+ mgr = resourceLoader.newInstance(restManagerPluginInfo, RestManager.class);
Review comment:
reload of packages not considered
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361021437
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -1408,7 +1415,7 @@ private Codec initCodec(SolrConfig solrConfig, final IndexSchema schema) {
final PluginInfo info = solrConfig.getPluginInfo(CodecFactory.class.getName());
final CodecFactory factory;
if (info != null) {
- factory = resourceLoader.newInstance(info.className, CodecFactory.class);
+ factory = resourceLoader.newInstance(info, CodecFactory.class);
Review comment:
one mor eplace where reload of packages are not considered at all
----------------------------------------------------------------
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 #1109: More pervasive
use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on issue #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-569156544
> I like the 3 categories of plugins very much, and also especially glad and surprised to hear you state "Hot reloading is not important."
The reason why I went with hot reloading for certain components was that it was easy to implement and tests were less likely to fail. My experience with `runtimeLib` taught me that core reloads are not very reliable during tests. They fail sporadically. Moreover, we should aim to reload the smallest possible entity for sake of efficiency and stability. For instance, reloading an instance of a `SearchComponent` may happen in sub milliseconds, but a core reload can take an unpredictable amount of time depending on various other factors
The idea of SRL resolving the class name to appropriate package works, but it's hard to get it right. Hot reloading is not the problem, consistency is. Consider the following
- packages can be updated any time
- cores can be reloaded any time
- nodes can be restarted at any time
The challenge is to ensure that every single plugin in every single node in the cluster has the same version of the package being used. If we cannot have a mechanism to somehow reload the plugin, we cannot ensure consistency. Every time the package is updated, we must ensure that the plugin is refreshed. SRL is generic. It doesn't know how to reload some plugin optimally.
Today, all the implementations treat the plugin itself as the atomic unit (even then there are 3 types : Search components, Streaming expressions and other components). #1124 treats the `IndexSchema` as the atomic unit. We may have another set of plugins that use the `SolrCore` as the atomic unit. There are so many ways we can do this. But, we CANNOT say that we will somehow load the classes today and we will worry about reloading it later. It works fine for a single node Solr , it cannot work correctly for a cluster of nodes. So, when I say something is "plain wrong", it just means that the design choices are making it impossible for us to give appropriate consistency guarantees for the entire system.
Then we have another often overlooked factor of "diagnosability". How do we give the user visibility into what version of the plugin is used in a given node/core? There should be an appropriate end point where people can probe the system to get that information.
----------------------------------------------------------------
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 removed a comment on issue #1109: More
pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul removed a comment on issue #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-568981311
@dsmiley The comments are not supposed to be personal. These are my observations on the code/PR. I'm sure somebody is going to remove that "WIP" tag pretty soon. There is a ton of work that needs to be done before we can do that. So, I'm typing down my observations as I see them.
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361333891
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##########
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className, IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info, IndexSchemaFactory.class);
Review comment:
Sound like an exciting design, SRL in response of loading stuff. Could not SRL ask SolrCore about version details if it is a core-level package?
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361021908
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -954,4 +987,46 @@ public static void persistConfLocally(SolrResourceLoader loader, String resource
}
}
+ // TODO document these methods...
Review comment:
What is the motivation behind `SolrResourceLoader` returning packages?
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361062854
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -810,28 +809,30 @@ void initIndex(boolean passOnPreviousState, boolean reload) throws IOException {
* Creates an instance by trying a constructor that accepts a SolrCore before
* trying the default (no arg) constructor.
*
- * @param className the instance class to create
+ * @param pluginInfo the instance class to create
* @param cast the class or interface that the instance should extend or implement
- * @param msg a message helping compose the exception error if any occurs.
* @param core The SolrCore instance for which this object needs to be loaded
* @return the desired instance
* @throws SolrException if the object could not be instantiated
*/
- public static <T> T createInstance(String className, Class<T> cast, String msg, SolrCore core, ResourceLoader resourceLoader) {
- Class<? extends T> clazz = null;
- if (msg == null) msg = "SolrCore Object";
+ public static <T> T newInstance(PluginInfo pluginInfo, Class<T> cast, SolrCore core, SolrResourceLoader resourceLoader) {
+ String msg = pluginInfo.type;
try {
- clazz = resourceLoader.findClass(className, cast);
- //most of the classes do not have constructors which takes SolrCore argument. It is recommended to obtain SolrCore by implementing SolrCoreAware.
- // So invariably always it will cause a NoSuchMethodException. So iterate though the list of available constructors
- Constructor<?>[] cons = clazz.getConstructors();
- for (Constructor<?> con : cons) {
- Class<?>[] types = con.getParameterTypes();
- if (types.length == 1 && types[0] == SolrCore.class) {
- return cast.cast(con.newInstance(core));
+ //TODO separate out "core" scenario to another method
+ if (pluginInfo.pkgName == null && core != null) {
+ Class<? extends T> clazz = resourceLoader.findClass(pluginInfo.className, cast);
+ //most of the classes do not have constructors which takes SolrCore argument. It is recommended to obtain SolrCore by implementing SolrCoreAware.
Review comment:
I changed this method on SolrCore (formerly createInstance, now newInstance) to take a PluginInfo. This is important because the caller often has one, and the PluginInfo has both the package and class name separated out. Previously, this method only took a String className but that value tended to be pluginInfo.className from the caller which thus stripped out the important information of which package it's on.
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361234358
##########
File path: solr/core/src/java/org/apache/solr/core/PluginBag.java
##########
@@ -140,11 +139,10 @@ public static void initInstance(Object inst, PluginInfo info) {
log.debug("{} : '{}' created with startup=lazy ", meta.getCleanTag(), info.name);
return new LazyPluginHolder<T>(meta, info, core, core.getResourceLoader(), false);
} else {
- if (info.pkgName != null) {
- PackagePluginHolder<T> holder = new PackagePluginHolder<>(info, core, meta);
- return holder;
+ if (core.getResourceLoader().getPackage(info.pkgName) != null) {
Review comment:
The idea of `SRL` trying to resolve the correct package is wrong. Let's not do this. SRL has no idea what version of a package should be used to load a plugin. Only a `SolrCore` does
----------------------------------------------------------------
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 #1109: More pervasive
use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on issue #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-569014248
> RE "inconsistent state": Put differently, I think you are just pointing out that this PR is not loading the correct version? Yes; this is a known blocker/nocommit. Did I mention this was a hackday project ;-)
>
> The latter part of your message seems to point to an as-yet unimplemented improvement proposal for the package management system (is this the right name?) to track which plugin abstractions are hot-loadable or not. I don't think that's in scope of this PR. BTW so that we don't confuse each other, I propose that "hot-loadable" imply _no_ core reload.
We should document clearly, which plugins are
- reloaded without reloading core
- which are hot loaded
- or somethings cannot even be reloaded and it will fail
Hot reloading is not important. But clearly documenting and setting expectations is important
I clearly understand this is a hackday PR. I have no complaints about it. You seem to have misconstrued by feedback as a criticism. I just wrote down my observations. (Some observations may even be wrong as I wouldn't have thoroughly gone through the huge PR).
We definitely want more review comments on our PRs and not less. So, let's make life a bit easier for the reviewers.
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361021700
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -529,6 +536,18 @@ public String resourceLocation(String resource) {
Class<? extends T> clazz = null;
try {
+ // If there is a package name prefix ...
+ Pair<String, String> pkgClassPair = PluginInfo.parseClassName(cname);
+ PackageLoader.Package pkg = getPackage(pkgClassPair.first());
+ if (pkg == null) {
+ // essentially, remove the package prefix and continue as normal. Maybe it'll be found.
+ cname = pkgClassPair.second();
+ } else {
+ // TODO what version?
Review comment:
Trying to load the latest always? Why? Need to revisit.
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361061609
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -810,28 +809,30 @@ void initIndex(boolean passOnPreviousState, boolean reload) throws IOException {
* Creates an instance by trying a constructor that accepts a SolrCore before
* trying the default (no arg) constructor.
*
- * @param className the instance class to create
+ * @param pluginInfo the instance class to create
* @param cast the class or interface that the instance should extend or implement
- * @param msg a message helping compose the exception error if any occurs.
* @param core The SolrCore instance for which this object needs to be loaded
* @return the desired instance
* @throws SolrException if the object could not be instantiated
*/
- public static <T> T createInstance(String className, Class<T> cast, String msg, SolrCore core, ResourceLoader resourceLoader) {
- Class<? extends T> clazz = null;
- if (msg == null) msg = "SolrCore Object";
+ public static <T> T newInstance(PluginInfo pluginInfo, Class<T> cast, SolrCore core, SolrResourceLoader resourceLoader) {
+ String msg = pluginInfo.type;
try {
- clazz = resourceLoader.findClass(className, cast);
- //most of the classes do not have constructors which takes SolrCore argument. It is recommended to obtain SolrCore by implementing SolrCoreAware.
- // So invariably always it will cause a NoSuchMethodException. So iterate though the list of available constructors
- Constructor<?>[] cons = clazz.getConstructors();
- for (Constructor<?> con : cons) {
- Class<?>[] types = con.getParameterTypes();
- if (types.length == 1 && types[0] == SolrCore.class) {
- return cast.cast(con.newInstance(core));
+ //TODO separate out "core" scenario to another method
+ if (pluginInfo.pkgName == null && core != null) {
+ Class<? extends T> clazz = resourceLoader.findClass(pluginInfo.className, cast);
Review comment:
Quite right Noble! In so doing, with low effort we enable all plugins to be loaded at runtime. "Hot update" is for another day.
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361062000
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -810,28 +809,30 @@ void initIndex(boolean passOnPreviousState, boolean reload) throws IOException {
* Creates an instance by trying a constructor that accepts a SolrCore before
* trying the default (no arg) constructor.
*
- * @param className the instance class to create
+ * @param pluginInfo the instance class to create
* @param cast the class or interface that the instance should extend or implement
- * @param msg a message helping compose the exception error if any occurs.
* @param core The SolrCore instance for which this object needs to be loaded
* @return the desired instance
* @throws SolrException if the object could not be instantiated
*/
- public static <T> T createInstance(String className, Class<T> cast, String msg, SolrCore core, ResourceLoader resourceLoader) {
- Class<? extends T> clazz = null;
- if (msg == null) msg = "SolrCore Object";
+ public static <T> T newInstance(PluginInfo pluginInfo, Class<T> cast, SolrCore core, SolrResourceLoader resourceLoader) {
+ String msg = pluginInfo.type;
try {
- clazz = resourceLoader.findClass(className, cast);
- //most of the classes do not have constructors which takes SolrCore argument. It is recommended to obtain SolrCore by implementing SolrCoreAware.
- // So invariably always it will cause a NoSuchMethodException. So iterate though the list of available constructors
- Constructor<?>[] cons = clazz.getConstructors();
- for (Constructor<?> con : cons) {
- Class<?>[] types = con.getParameterTypes();
- if (types.length == 1 && types[0] == SolrCore.class) {
- return cast.cast(con.newInstance(core));
+ //TODO separate out "core" scenario to another method
+ if (pluginInfo.pkgName == null && core != null) {
+ Class<? extends T> clazz = resourceLoader.findClass(pluginInfo.className, cast);
Review comment:
RE SolrCore.newInstance: II renamed SolrCore.createInstance to SolrCore.newInstance so to be more consistent with ResourceLoader.newInstance. Also I changed the signature of it a bit... I have some TODOs there. I'm kinda dubious about that method on SolrCore... seems could be dropped and just use ResourceLoader.newInstance however I'm not sure how to handle plugins that take a SolrCore in its 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 commented on a change in pull request
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361375529
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##########
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className, IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info, IndexSchemaFactory.class);
Review comment:
> There is no right/wrong here; we are exploring what the future holds together; it has not been written yet with no spec to judge right/wrong
"SRL has no idea which version of package should be loaded".
This was my statement. I never said SRL loading classes is "plain wrong". According to the PR it was loading the latest version of the package and "that is wrong". Am I supposed to comment on what the PR will look like in the future or am I supposed to make comments on what it does today?
I think you are working during the holidays and you are not reading the comments fully
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361061471
##########
File path: solr/contrib/velocity/src/java/org/apache/solr/response/VelocityResponseWriter.java
##########
@@ -275,7 +276,7 @@ private VelocityContext createContext(SolrQueryRequest request, SolrQueryRespons
for (Map.Entry<String, String> entry : customTools.entrySet()) {
String name = entry.getKey();
// TODO: at least log a warning when one of the *fixed* tools classes is same name with a custom one, currently silently ignored
- Object customTool = SolrCore.createInstance(entry.getValue(), Object.class, "VrW custom tool: " + name, request.getCore(), request.getCore().getResourceLoader());
+ Object customTool = SolrCore.newInstance(new PluginInfo(name, entry.getValue()), Object.class, request.getCore(), request.getCore().getResourceLoader());
Review comment:
I'll add a code review comment over on that method in SolrCore so we can discuss it in context; I renamed it. What is the correct classloader it should use?
----------------------------------------------------------------
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 #1109: More pervasive
use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on issue #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-568812967
Technically, Hot-reloading means reloading an object without restarting the JVM. So, it's OK to reload an object by reloading a core.
The problem with this PR is that it leaves the system in an inconsistent state. Consider the following scenario
- `ComponentX` uses `PackageA` `1.0`
- User updates `PackageA` to `2.0`
- A core that got started after the package update uses `2.0` & all other cores use `1.0`
We really cannot leave the system in an inconsistent state and say we will deal with the inconsistencies in another ticket.
What is the solution?
There are 2 ways to use packages.
1. Ensure that the plugin has true hot reloading. As it is done for the plugins we support today
2. Or, we identify a list of plugins that do not need to be `hot-reloaded`. If a package is updated and such a plugin is using that package, we just reload the core
Wherever possible we strive to implement option#1 because reloading cores can cause instability in the system & we try to minimize it as much as possible. If option#1 is not possible we MUST implement option#2.
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361064421
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##########
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className, IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info, IndexSchemaFactory.class);
Review comment:
> Do we even support the packageName:ClassName scheme in schema.xml at all?
It does! Woohoo! It's because all loading occurs via a SolrResourceLoader in Solr and so that's where the focal point of the approach in this PR is.
----------------------------------------------------------------
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 #1109: More pervasive
use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on issue #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-568981311
@dsmiley The comments are not supposed to be personal. These are my observations on the code/PR. I'm sure somebody is going to remove that "WIP" tag pretty soon. There is a ton of work that needs to be done before we can do that. So, I'm typing down my observations as I see them.
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361022038
##########
File path: solr/core/src/java/org/apache/solr/metrics/MetricSuppliers.java
##########
@@ -283,7 +283,7 @@ public Histogram newMetric() {
MetricRegistry.MetricSupplier<Counter> supplier;
try {
- supplier = loader.newInstance(info.className, MetricRegistry.MetricSupplier.class);
+ supplier = loader.newInstance(info, MetricRegistry.MetricSupplier.class);
Review comment:
How do we reload this?
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361366362
##########
File path: solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
##########
@@ -420,7 +420,7 @@ static DirectoryFactory loadDirectoryFactory(SolrConfig config, CoreContainer cc
final DirectoryFactory dirFactory;
if (info != null) {
log.debug(info.className);
- dirFactory = config.getResourceLoader().newInstance(info.className, DirectoryFactory.class);
+ dirFactory = config.getResourceLoader().newInstance(info, DirectoryFactory.class);
Review comment:
IndexSchema can be shared ("shareSchema" option) but SolrConfig at present cannot be. Lets suppose it can be for the sake of argument -- wouldn't this be a reason to _not_ use SolrCore related things? Otherwise another Collection using the same SolrConfig might theoretically refer to something it shouldn't.
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361233110
##########
File path: solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
##########
@@ -420,7 +420,7 @@ static DirectoryFactory loadDirectoryFactory(SolrConfig config, CoreContainer cc
final DirectoryFactory dirFactory;
if (info != null) {
log.debug(info.className);
- dirFactory = config.getResourceLoader().newInstance(info.className, DirectoryFactory.class);
+ dirFactory = config.getResourceLoader().newInstance(info, DirectoryFactory.class);
Review comment:
It's possible that we can share instances of SolrConfig between cores. We should always use SRL from 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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361401294
##########
File path: solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
##########
@@ -420,7 +420,7 @@ static DirectoryFactory loadDirectoryFactory(SolrConfig config, CoreContainer cc
final DirectoryFactory dirFactory;
if (info != null) {
log.debug(info.className);
- dirFactory = config.getResourceLoader().newInstance(info.className, DirectoryFactory.class);
+ dirFactory = config.getResourceLoader().newInstance(info, DirectoryFactory.class);
Review comment:
yeah, schema is shared. SolrConfig is not. Maybe it's safe
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361490021
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##########
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className, IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info, IndexSchemaFactory.class);
Review comment:
> am I supposed to make comments on what it does today?
Yes (agreed); and not the future.
> I never said SRL loading classes is "plain wrong
I'm glad to hear that. You made this statement immediately after what I said, which was "It does! Woohoo! It's because all loading occurs via a SolrResourceLoader in Solr and so that's where the focal point of the approach in this PR is." So I thought you meant my most recent statement "was plain wrong" vs some statement in some other comment thread (notice this thread doesn't discuss versions). Can you see how I came to this interpretation?
I am working this week, albeit lightly BTW.
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361064071
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -954,4 +987,46 @@ public static void persistConfLocally(SolrResourceLoader loader, String resource
}
}
+ // TODO document these methods...
Review comment:
I came to think of the SolrResourceLoader as being rather related to packages. SRL has methods that lookup class names to actual Classes and then instances, so the focal point of my approach is to have `findClass` here work with the package system.
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361020495
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -519,7 +518,7 @@ private IndexDeletionPolicyWrapper initDeletionPolicy(IndexDeletionPolicyWrapper
final PluginInfo info = solrConfig.getPluginInfo(IndexDeletionPolicy.class.getName());
final IndexDeletionPolicy delPolicy;
if (info != null) {
- delPolicy = createInstance(info.className, IndexDeletionPolicy.class, "Deletion Policy for SOLR", this, getResourceLoader());
+ delPolicy = newInstance(info, IndexDeletionPolicy.class, this, getResourceLoader());
Review comment:
Looks wrong. Shouldn't we get the correct Package classloader 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] dsmiley commented on issue #1109: More pervasive use
of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on issue #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#issuecomment-568654612
I think one of the most important next steps for Solr is that it support the vast majority of plugin abstractions, and that is the goal of this PR. For example the schema's abstractions are super important. I don't know what it takes for a plugin abstraction to support hot-loading; I deliberately overlooked that. Can they both be done easily at the same time? If not I suggest a separating that out.
I know there is no test here yet. I had something very WIP I didn't commit but proved very useful in trying out the schema. I had it in `TestPackages`; does that seem suitable? I wonder if we should have a test configSet in which _every_ plugin specification point is in its own package as a way of testing total coverage. WDYT?
One thing *not* working that is important is lifecycle methods like `SolrCoreAware`; maybe others.
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361063126
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -890,7 +897,7 @@ private UpdateHandler createReloadedUpdateHandler(String className, String msg,
}
private UpdateHandler createUpdateHandler(String className) {
- return createInstance(className, UpdateHandler.class, "Update Handler", this, getResourceLoader());
+ return newInstance(new PluginInfo("updateHandler", className), UpdateHandler.class, this, getResourceLoader());
Review comment:
Yes, I strongly believe hot-updates should occur in a separate ticket given that with relative ease we can get all plugins to load via the amazing plugin system :-)
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361018813
##########
File path: solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
##########
@@ -420,7 +420,7 @@ static DirectoryFactory loadDirectoryFactory(SolrConfig config, CoreContainer cc
final DirectoryFactory dirFactory;
if (info != null) {
log.debug(info.className);
- dirFactory = config.getResourceLoader().newInstance(info.className, DirectoryFactory.class);
+ dirFactory = config.getResourceLoader().newInstance(info, DirectoryFactory.class);
Review comment:
Whats the point of this call? We should never use `config.getResourceLoader()`
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361367240
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -954,4 +987,46 @@ public static void persistConfLocally(SolrResourceLoader loader, String resource
}
}
+ // TODO document these methods...
Review comment:
The SRL is pervasively used across the codebase to load stuff (hence "resource loader" in its name). If CoreContainer/SolrCore is now the right place, then this is a big change; no?
BTW, Wouldn't the ConfigSet object be a more befitting place to store the version association than the SolrCore? Isn't params.json (which is where the association is) stored in the config set?
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361021009
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -810,28 +809,30 @@ void initIndex(boolean passOnPreviousState, boolean reload) throws IOException {
* Creates an instance by trying a constructor that accepts a SolrCore before
* trying the default (no arg) constructor.
*
- * @param className the instance class to create
+ * @param pluginInfo the instance class to create
* @param cast the class or interface that the instance should extend or implement
- * @param msg a message helping compose the exception error if any occurs.
* @param core The SolrCore instance for which this object needs to be loaded
* @return the desired instance
* @throws SolrException if the object could not be instantiated
*/
- public static <T> T createInstance(String className, Class<T> cast, String msg, SolrCore core, ResourceLoader resourceLoader) {
- Class<? extends T> clazz = null;
- if (msg == null) msg = "SolrCore Object";
+ public static <T> T newInstance(PluginInfo pluginInfo, Class<T> cast, SolrCore core, SolrResourceLoader resourceLoader) {
+ String msg = pluginInfo.type;
try {
- clazz = resourceLoader.findClass(className, cast);
- //most of the classes do not have constructors which takes SolrCore argument. It is recommended to obtain SolrCore by implementing SolrCoreAware.
- // So invariably always it will cause a NoSuchMethodException. So iterate though the list of available constructors
- Constructor<?>[] cons = clazz.getConstructors();
- for (Constructor<?> con : cons) {
- Class<?>[] types = con.getParameterTypes();
- if (types.length == 1 && types[0] == SolrCore.class) {
- return cast.cast(con.newInstance(core));
+ //TODO separate out "core" scenario to another method
+ if (pluginInfo.pkgName == null && core != null) {
+ Class<? extends T> clazz = resourceLoader.findClass(pluginInfo.className, cast);
Review comment:
The repeated pattern I see is, The patch only looks at how to load the class when the core is loaded. Pays no attention to how to reload it, if the package is updated.
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361062261
##########
File path: solr/core/src/java/org/apache/solr/core/DirectoryFactory.java
##########
@@ -420,7 +420,7 @@ static DirectoryFactory loadDirectoryFactory(SolrConfig config, CoreContainer cc
final DirectoryFactory dirFactory;
if (info != null) {
log.debug(info.className);
- dirFactory = config.getResourceLoader().newInstance(info.className, DirectoryFactory.class);
+ dirFactory = config.getResourceLoader().newInstance(info, DirectoryFactory.class);
Review comment:
Why? FYI the ResourceLoader on the config is the same as that of the core, I believe.
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361019528
##########
File path: solr/core/src/java/org/apache/solr/core/PluginBag.java
##########
@@ -140,11 +139,10 @@ public static void initInstance(Object inst, PluginInfo info) {
log.debug("{} : '{}' created with startup=lazy ", meta.getCleanTag(), info.name);
return new LazyPluginHolder<T>(meta, info, core, core.getResourceLoader(), false);
} else {
- if (info.pkgName != null) {
- PackagePluginHolder<T> holder = new PackagePluginHolder<>(info, core, meta);
- return holder;
+ if (core.getResourceLoader().getPackage(info.pkgName) != null) {
Review comment:
Shouldn't we fail fast instead of continuing as if it there is no problem?
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361063486
##########
File path: solr/core/src/java/org/apache/solr/core/SolrResourceLoader.java
##########
@@ -529,6 +536,18 @@ public String resourceLocation(String resource) {
Class<? extends T> clazz = null;
try {
+ // If there is a package name prefix ...
+ Pair<String, String> pkgClassPair = PluginInfo.parseClassName(cname);
+ PackageLoader.Package pkg = getPackage(pkgClassPair.first());
+ if (pkg == null) {
+ // essentially, remove the package prefix and continue as normal. Maybe it'll be found.
+ cname = pkgClassPair.second();
+ } else {
+ // TODO what version?
Review comment:
Yes, I have a "TODO" at this line. How do I get the right version?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361021135
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -810,28 +809,30 @@ void initIndex(boolean passOnPreviousState, boolean reload) throws IOException {
* Creates an instance by trying a constructor that accepts a SolrCore before
* trying the default (no arg) constructor.
*
- * @param className the instance class to create
+ * @param pluginInfo the instance class to create
* @param cast the class or interface that the instance should extend or implement
- * @param msg a message helping compose the exception error if any occurs.
* @param core The SolrCore instance for which this object needs to be loaded
* @return the desired instance
* @throws SolrException if the object could not be instantiated
*/
- public static <T> T createInstance(String className, Class<T> cast, String msg, SolrCore core, ResourceLoader resourceLoader) {
- Class<? extends T> clazz = null;
- if (msg == null) msg = "SolrCore Object";
+ public static <T> T newInstance(PluginInfo pluginInfo, Class<T> cast, SolrCore core, SolrResourceLoader resourceLoader) {
+ String msg = pluginInfo.type;
try {
- clazz = resourceLoader.findClass(className, cast);
- //most of the classes do not have constructors which takes SolrCore argument. It is recommended to obtain SolrCore by implementing SolrCoreAware.
- // So invariably always it will cause a NoSuchMethodException. So iterate though the list of available constructors
- Constructor<?>[] cons = clazz.getConstructors();
- for (Constructor<?> con : cons) {
- Class<?>[] types = con.getParameterTypes();
- if (types.length == 1 && types[0] == SolrCore.class) {
- return cast.cast(con.newInstance(core));
+ //TODO separate out "core" scenario to another method
+ if (pluginInfo.pkgName == null && core != null) {
+ Class<? extends T> clazz = resourceLoader.findClass(pluginInfo.className, cast);
+ //most of the classes do not have constructors which takes SolrCore argument. It is recommended to obtain SolrCore by implementing SolrCoreAware.
Review comment:
This has nothing to do with package loader. Can be a separate ticket
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361022358
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##########
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className, IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info, IndexSchemaFactory.class);
Review comment:
Do we even support the `packageName:ClassName` scheme in `schema.xml` at all? How does it play with this?
Need more discussions
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361586269
##########
File path: solr/core/src/java/org/apache/solr/core/PluginBag.java
##########
@@ -140,11 +139,10 @@ public static void initInstance(Object inst, PluginInfo info) {
log.debug("{} : '{}' created with startup=lazy ", meta.getCleanTag(), info.name);
return new LazyPluginHolder<T>(meta, info, core, core.getResourceLoader(), false);
} else {
- if (info.pkgName != null) {
- PackagePluginHolder<T> holder = new PackagePluginHolder<>(info, core, meta);
- return holder;
+ if (core.getResourceLoader().getPackage(info.pkgName) != null) {
Review comment:
> The idea of SRL trying to resolve the correct package is wrong.
You seem pretty adamant about this; I suggest saying you don't like that approach instead of it being "wrong". In this PR, SRL resolving the package is a central theme and so if we don't agree on wether SRL should do this then this PR has no future (ah well). Maybe we will return to this idea if there are holes in other approaches.
I view the lack of correct version resolution in the PR (still only one commit as I write this) as merely a necessary TODO, and I don't forsee it'd be hard. If you would like me to explore adding this next, let me know.
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361367530
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##########
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className, IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info, IndexSchemaFactory.class);
Review comment:
"That's plain wrong. " Please work on your tone @noblepaul . There is no right/wrong here; we are exploring what the future holds together; it has not been written yet with no spec to judge right/wrong. It would be more correct to say "According to my vision, the SRL should not know about packages".
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361021936
##########
File path: solr/core/src/java/org/apache/solr/core/TracerConfigurator.java
##########
@@ -43,8 +43,7 @@ public static void loadTracer(SolrResourceLoader loader, PluginInfo info, ZkStat
GlobalTracer.get().setSamplePercentage(0.0);
}
} else {
- TracerConfigurator configurator = loader
- .newInstance(info.className, TracerConfigurator.class);
+ TracerConfigurator configurator = loader.newInstance(info, TracerConfigurator.class);
Review comment:
no , thought given to reload
----------------------------------------------------------------
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
#1109: More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361375529
##########
File path: solr/core/src/java/org/apache/solr/schema/IndexSchemaFactory.java
##########
@@ -62,7 +62,7 @@ public static IndexSchema buildIndexSchema(String resourceName, SolrConfig confi
PluginInfo info = config.getPluginInfo(IndexSchemaFactory.class.getName());
IndexSchemaFactory factory;
if (null != info) {
- factory = config.getResourceLoader().newInstance(info.className, IndexSchemaFactory.class);
+ factory = config.getResourceLoader().newInstance(info, IndexSchemaFactory.class);
Review comment:
> There is no right/wrong here;
This is not a personal remark. What I'm saying is "it's plain wrong to load using the latest version".
This is not how the rest of the system work. So, we have to ensure that what should be the expectation of the user
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361366578
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -519,7 +518,7 @@ private IndexDeletionPolicyWrapper initDeletionPolicy(IndexDeletionPolicyWrapper
final PluginInfo info = solrConfig.getPluginInfo(IndexDeletionPolicy.class.getName());
final IndexDeletionPolicy delPolicy;
if (info != null) {
- delPolicy = createInstance(info.className, IndexDeletionPolicy.class, "Deletion Policy for SOLR", this, getResourceLoader());
+ delPolicy = newInstance(info, IndexDeletionPolicy.class, this, getResourceLoader());
Review comment:
This ultimately calls SRL with PackageInfo which resolves the right package classloader there. That's the central idea of the approach in this PR -- SRL owns the resolution.
----------------------------------------------------------------
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 #1109:
More pervasive use of PackageLoader / PluginInfo
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1109: More pervasive use of PackageLoader / PluginInfo
URL: https://github.com/apache/lucene-solr/pull/1109#discussion_r361062164
##########
File path: solr/core/src/java/org/apache/solr/core/PluginBag.java
##########
@@ -140,11 +139,10 @@ public static void initInstance(Object inst, PluginInfo info) {
log.debug("{} : '{}' created with startup=lazy ", meta.getCleanTag(), info.name);
return new LazyPluginHolder<T>(meta, info, core, core.getResourceLoader(), false);
} else {
- if (info.pkgName != null) {
- PackagePluginHolder<T> holder = new PackagePluginHolder<>(info, core, meta);
- return holder;
+ if (core.getResourceLoader().getPackage(info.pkgName) != null) {
Review comment:
that method SolrResourceLoader.getPackage will fail fast it can't load it for whatever reason. See `handleUnresolvedPackage` which it calls.
----------------------------------------------------------------
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