Skip to content
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

Merged
merged 8 commits into from
Jun 2, 2020

Conversation

noblepaul
Copy link
Contributor

SOLR-14525
For components loaded from packages SolrCoreAware, ResourceLoaderAware are not honored

Copy link
Contributor

@dsmiley dsmiley left a 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.

solr/core/src/java/org/apache/solr/pkg/PackageLoader.java Outdated Show resolved Hide resolved
@Override
public <T> boolean addToCoreAware(T obj) {
//do not do anything
//this class is
Copy link
Contributor

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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

  1. create instance
  2. init()
  3. 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha; makes sense.

@noblepaul noblepaul self-assigned this Jun 1, 2020
@noblepaul noblepaul merged commit e841d76 into master Jun 2, 2020
@noblepaul noblepaul deleted the jira/solr14525 branch June 25, 2020 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants