Solr
  1. Solr
  2. SOLR-260

reusable PluginLoader -- helper class to load plugins

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.3
    • Component/s: None
    • Labels:
      None

      Description

      As we talk about adding more configuration (Handlers, Highlighting, Components, etc) we should standardize the format and share the loading and initialization code.

      This patch extracts the common stuff from SOLR-225 and makes it work with the RequestHandler framework.

      This is an abstract base class – each implementation needs to take care of actually creating and initializing the instances:

      abstract class PluginLoader<T>
      {
      abstract public T create( String className, NamedList args, Map<String,String> params );

      abstract public void init( T plugin, NamedList args, Map<String,String> params );

      public Map<String,T> load( NodeList nodes )

      { ... }

      }

      1. SOLR-260-PluginLoader.patch
        31 kB
        Ryan McKinley
      2. SOLR-260-PluginLoader.patch
        31 kB
        Ryan McKinley

        Issue Links

          Activity

          Hide
          Hoss Man added a comment -

          Ryan: FYI but there is currently no patch attached to this issue.

          looking ta the way PluginLoader is used in the latest SOLR-225 patch, i wonder if some of this stuff could be simplified more by adding two new super interfaces to many of the things that can currently be specified as plugins (you alluded to this in an old comment in SOLR-225 that i remember seeing when skimming my email a while back, but never really got a chance to comment on).

          Assuming we have two interfaces like this...

          public interface MapInitializedPlugin

          { init( Map<String,String> args); }

          public interface NamedListInitializedPlugin

          { init( NamedList args ); }

          ...then the PluginLoader concept can be much simpler can't it? something like...

          public abstract class AbstractPluginLoader<T> {
          protected T createInstance(String pluginClassName)

          { Class clazz = Config.findClass( pluginClassName, DEFAULT_PACAKGES); return (T) clazz.newInstance(); // :TODO: do some error check that clazz extends T }

          public class MapPluginLoader<T extends MapInitializedPlugin> extends AbstractPluginLoader<T> {
          protected T createInstance(String pluginClassName, Map<String,String> args)

          { createInstance( pluginClassName); }
          public T getInstance(String pluginClassName, Map<String,String> args) { T plugin = createInstance(pluginClassName); plugin.init(args); return plugin; }
          }
          public class NamedListPluginLoader<T extends NamedListInitializedPlugin> extends AbstractPluginLoader<T> {
          protected T createInstance(String pluginClassName, NamedList args) { createInstance( pluginClassName); }

          public T getInstance(String pluginClassName, NamedList args)

          { T plugin = createInstance(pluginClassName); plugin.init(args); return plugin; }

          }

          ...similar batch processing methods to the ones you already have (for dealing with DOM Nodes) can also be added, but the key is that you can say things like...

          SolrRequestHandler r = (new NamedLIstPluginLoader<SolrRequestHandler>()).getInstance(concreteImplName);

          ...instead of each usage needing to provide an impl for the PluginLoader.init method (and the methods don't all have the double usage of NamedList and Map<String,String> args that might confuse people since one of them is always ignored).

          NOTE: I'm not sure why you have your PluginLoader.create method abstract ... it seems like the common case is common enough that we can provide a default impl.

          Show
          Hoss Man added a comment - Ryan: FYI but there is currently no patch attached to this issue. looking ta the way PluginLoader is used in the latest SOLR-225 patch, i wonder if some of this stuff could be simplified more by adding two new super interfaces to many of the things that can currently be specified as plugins (you alluded to this in an old comment in SOLR-225 that i remember seeing when skimming my email a while back, but never really got a chance to comment on). Assuming we have two interfaces like this... public interface MapInitializedPlugin { init( Map<String,String> args); } public interface NamedListInitializedPlugin { init( NamedList args ); } ...then the PluginLoader concept can be much simpler can't it? something like... public abstract class AbstractPluginLoader<T> { protected T createInstance(String pluginClassName) { Class clazz = Config.findClass( pluginClassName, DEFAULT_PACAKGES); return (T) clazz.newInstance(); // :TODO: do some error check that clazz extends T } public class MapPluginLoader<T extends MapInitializedPlugin> extends AbstractPluginLoader<T> { protected T createInstance(String pluginClassName, Map<String,String> args) { createInstance( pluginClassName); } public T getInstance(String pluginClassName, Map<String,String> args) { T plugin = createInstance(pluginClassName); plugin.init(args); return plugin; } } public class NamedListPluginLoader<T extends NamedListInitializedPlugin> extends AbstractPluginLoader<T> { protected T createInstance(String pluginClassName, NamedList args) { createInstance( pluginClassName); } public T getInstance(String pluginClassName, NamedList args) { T plugin = createInstance(pluginClassName); plugin.init(args); return plugin; } } ...similar batch processing methods to the ones you already have (for dealing with DOM Nodes) can also be added, but the key is that you can say things like... SolrRequestHandler r = (new NamedLIstPluginLoader<SolrRequestHandler>()).getInstance(concreteImplName); ...instead of each usage needing to provide an impl for the PluginLoader.init method (and the methods don't all have the double usage of NamedList and Map<String,String> args that might confuse people since one of them is always ignored). NOTE: I'm not sure why you have your PluginLoader.create method abstract ... it seems like the common case is common enough that we can provide a default impl.
          Hide
          Hoss Man added a comment -

          a few more thoughts...
          1) instead of DEFAULT_PACAKGES that should really be "String[] getDefaultPackages()" method that could be overridden in subclasses (anonymous or otherwise)
          2) there's really no reason for the createInstance methods i put in NamedListPluginLoader and MapPluginLoader ... if a special case comes along where a special type of plugin requires a special type of instantiation, people can take care of it by overriding getInstance.

          Show
          Hoss Man added a comment - a few more thoughts... 1) instead of DEFAULT_PACAKGES that should really be "String[] getDefaultPackages()" method that could be overridden in subclasses (anonymous or otherwise) 2) there's really no reason for the createInstance methods i put in NamedListPluginLoader and MapPluginLoader ... if a special case comes along where a special type of plugin requires a special type of instantiation, people can take care of it by overriding getInstance.
          Hide
          Ryan McKinley added a comment -

          >
          > Ryan: FYI but there is currently no patch attached to this issue.
          >

          i'm trying to mush RequestHandlers and FieldTypes into it now I'll post something soon – integrating a few of your thoughts.

          Show
          Ryan McKinley added a comment - > > Ryan: FYI but there is currently no patch attached to this issue. > i'm trying to mush RequestHandlers and FieldTypes into it now I'll post something soon – integrating a few of your thoughts.
          Hide
          Ryan McKinley added a comment -

          This patch uses a single plugin loader for:

          • RequestHandlers
          • FieldTypes
          • QueryResponseWriter
          • (SOLR-225 highlighting)

          It could be used for field and dynamicField, but I'll hold off on that...

          RequestHandlers and FieldTypes have enough custom stuff going on (lazy loading, Analyzers) that they can't use the general class NamedListPluginLoader<T>

          QueryResponseWriters are initalized with:

          NamedListPluginLoader<QueryResponseWriter> loader =
          new NamedListPluginLoader<QueryResponseWriter>( "[solrconfig.xml] "+xpath, responseWriters );
          defaultResponseWriter = loader.load( nodes );

          • - -

          This configuration adds the ability to specify the default RequestHandlers/QueryResponseWriter using default="true" on exactly one entry. This was required for SOLR-225 highlighting config, but I think it is generally useful.

          • - -

          I added package o.a.solr.util.plugin with 5 classes. Any better suggestions where this ought to live?

          Show
          Ryan McKinley added a comment - This patch uses a single plugin loader for: RequestHandlers FieldTypes QueryResponseWriter ( SOLR-225 highlighting) It could be used for field and dynamicField, but I'll hold off on that... RequestHandlers and FieldTypes have enough custom stuff going on (lazy loading, Analyzers) that they can't use the general class NamedListPluginLoader<T> QueryResponseWriters are initalized with: NamedListPluginLoader<QueryResponseWriter> loader = new NamedListPluginLoader<QueryResponseWriter>( " [solrconfig.xml] "+xpath, responseWriters ); defaultResponseWriter = loader.load( nodes ); - - This configuration adds the ability to specify the default RequestHandlers/QueryResponseWriter using default="true" on exactly one entry. This was required for SOLR-225 highlighting config, but I think it is generally useful. - - I added package o.a.solr.util.plugin with 5 classes. Any better suggestions where this ought to live?
          Hide
          Ryan McKinley added a comment -

          No real changes... applies with trunk

          Show
          Ryan McKinley added a comment - No real changes... applies with trunk
          Hide
          Ryan McKinley added a comment -

          The most recent patch in SOLR-225 includes this change. If you want to evaluate the plugin architecture, check there – the patch on this issue will be slightly out date...

          Show
          Ryan McKinley added a comment - The most recent patch in SOLR-225 includes this change. If you want to evaluate the plugin architecture, check there – the patch on this issue will be slightly out date...
          Hide
          Hoss Man added a comment -

          Ryan ... sorry i slacked off on reviewing the last few patches in this issue. i just took a look at commit #552680 and it looks sweet. I'm still not crazy about the "init(T plugin, Map<String, String> params, Node node)" method sig, but since we have to get the class/name params anyway, i guess it doesn't do any harm to pass the Map in along with the Node ... we just need to beef up the javadocs so it's clear where the Map comes from

          Just to clarify, if i'm understanding the default="true" code, it means that RequestHandler resolution will go as follows...
          1) look for a RequestHandler with a name matching the request
          2) look for a RequestHandler configured with default="true"
          3) look for a RequestHandler configured with name="standard"
          4) use an anonymous instance of StandardRequestHandler

          ...that looks good to me, but we should definitely document it.

          Minor Question: isn't the init method in the AbstractPluginLoader<SolrRequestHandler> redundant?

          Show
          Hoss Man added a comment - Ryan ... sorry i slacked off on reviewing the last few patches in this issue. i just took a look at commit #552680 and it looks sweet. I'm still not crazy about the "init(T plugin, Map<String, String> params, Node node)" method sig, but since we have to get the class/name params anyway, i guess it doesn't do any harm to pass the Map in along with the Node ... we just need to beef up the javadocs so it's clear where the Map comes from Just to clarify, if i'm understanding the default="true" code, it means that RequestHandler resolution will go as follows... 1) look for a RequestHandler with a name matching the request 2) look for a RequestHandler configured with default="true" 3) look for a RequestHandler configured with name="standard" 4) use an anonymous instance of StandardRequestHandler ...that looks good to me, but we should definitely document it. Minor Question: isn't the init method in the AbstractPluginLoader<SolrRequestHandler> redundant?
          Hide
          Ryan McKinley added a comment -

          > I'm still not crazy about the "init(T plugin, Map<String, String> params, Node node)"

          I don't like it either. In 552709 I changed it to:
          init(T plugin, Node node)
          The few places that actually use the map can generate it using DOMUtils.

          > Just to clarify, if i'm understanding the default="true" code, it means that RequestHandler resolution will go as follows...
          > 1) look for a RequestHandler with a name matching the request
          > 2) look for a RequestHandler configured with default="true"
          > 3) look for a RequestHandler configured with name="standard"
          > 4) use an anonymous instance of StandardRequestHandler
          >

          exactly. likewise, the QueryResponseWriter follows the same pattern.

          > ...that looks good to me, but we should definitely document it.
          >

          Where do you think is the best place? the example solrconfig.xml?

          > Minor Question: isn't the init method in the AbstractPluginLoader<SolrRequestHandler> redundant?
          >

          I don't think so (but I am often wrong). If the RequestHandler plugin loader extended NamedListPluginLoader, then it would be. The RequestHandler funny business with lazy loading makes it better to directly subclass AbstractPluginLoader

          Show
          Ryan McKinley added a comment - > I'm still not crazy about the "init(T plugin, Map<String, String> params, Node node)" I don't like it either. In 552709 I changed it to: init(T plugin, Node node) The few places that actually use the map can generate it using DOMUtils. > Just to clarify, if i'm understanding the default="true" code, it means that RequestHandler resolution will go as follows... > 1) look for a RequestHandler with a name matching the request > 2) look for a RequestHandler configured with default="true" > 3) look for a RequestHandler configured with name="standard" > 4) use an anonymous instance of StandardRequestHandler > exactly. likewise, the QueryResponseWriter follows the same pattern. > ...that looks good to me, but we should definitely document it. > Where do you think is the best place? the example solrconfig.xml? > Minor Question: isn't the init method in the AbstractPluginLoader<SolrRequestHandler> redundant? > I don't think so (but I am often wrong). If the RequestHandler plugin loader extended NamedListPluginLoader, then it would be. The RequestHandler funny business with lazy loading makes it better to directly subclass AbstractPluginLoader
          Hide
          Hoss Man added a comment -

          > Where do you think is the best place? the example solrconfig.xml?

          well, at a minimum we should change it to use default="true" and document what that means, removing the comment about "standard" from there would probably make sense since new users aren't going to care about the magic of hte name "standard" with the default=true syntax .. but we should still document on the wiki somewhere what the full handler/writer resolution chain is.

          > The RequestHandler funny business with lazy loading makes it better to directly subclass AbstractPluginLoader

          ...ah, yeah i missed that ... but i don't see any reason why it can't still subclass NamedListPluginLoader just for the init method (even if it is does need custom create/register methods.

          Show
          Hoss Man added a comment - > Where do you think is the best place? the example solrconfig.xml? well, at a minimum we should change it to use default="true" and document what that means, removing the comment about "standard" from there would probably make sense since new users aren't going to care about the magic of hte name "standard" with the default=true syntax .. but we should still document on the wiki somewhere what the full handler/writer resolution chain is. > The RequestHandler funny business with lazy loading makes it better to directly subclass AbstractPluginLoader ...ah, yeah i missed that ... but i don't see any reason why it can't still subclass NamedListPluginLoader just for the init method (even if it is does need custom create/register methods.
          Hide
          Hoss Man added a comment -

          Ryan: do you consider this issue fixed, or did you want to leave it open to track converting more plugins to using PluginLoader? (if so we should probably list the classes in the issue description, if not we should open seperate bugs to track the individual types of plugins)

          Show
          Hoss Man added a comment - Ryan: do you consider this issue fixed, or did you want to leave it open to track converting more plugins to using PluginLoader? (if so we should probably list the classes in the issue description, if not we should open seperate bugs to track the individual types of plugins)
          Hide
          Ryan McKinley added a comment -

          should have been resolved a while ago.

          Show
          Ryan McKinley added a comment - should have been resolved a while ago.
          Hide
          Hoss Man added a comment -

          This bug was modified as part of a bulk update using the criteria...

          • Marked "Resolved" and "Fixed"
          • Had no "Fix Version" versions
          • Was listed in the CHANGES.txt for 1.3 as of today 2008-03-15

          The Fix Version for all 29 issues found was set to 1.3, email notification was suppressed to prevent excessive email.

          For a list of all the issues modified, search jira comments for this (hopefully) unique string: batch20070315hossman1

          Show
          Hoss Man added a comment - This bug was modified as part of a bulk update using the criteria... Marked "Resolved" and "Fixed" Had no "Fix Version" versions Was listed in the CHANGES.txt for 1.3 as of today 2008-03-15 The Fix Version for all 29 issues found was set to 1.3, email notification was suppressed to prevent excessive email. For a list of all the issues modified, search jira comments for this (hopefully) unique string: batch20070315hossman1

            People

            • Assignee:
              Ryan McKinley
              Reporter:
              Ryan McKinley
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development