-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SOLR-14525 For components loaded from packages SolrCoreAware, ResourceLoaderAware are not honored #1547
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this Noble! This was a big limitation. I have some small questions/feedback but mostly looks fine.
@Override | ||
public <T> boolean addToCoreAware(T obj) { | ||
//do not do anything | ||
//this class is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incomplete comment? Could use an explanation as to why. SolrCoreAware isn't handled here.
if (obj instanceof ResourceLoaderAware) { | ||
assertAwareCompatibility(ResourceLoaderAware.class, obj); | ||
try { | ||
((ResourceLoaderAware) obj).inform(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the order has to be
- create instance
init()
SolrCoreAware.inform()
,ResourceLoaderAware.inform()
I've made the relevant changes. Pls review
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha; makes sense.
This reverts commit 4ef305b. reverting last commit
SOLR-14525
For components loaded from packages SolrCoreAware, ResourceLoaderAware are not honored