Solr
  1. Solr
  2. SOLR-1106

Pluggable CoreAdminHandler (Action ) architecture that allows for custom handler access to CoreContainer / request-response

    Details

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

      Java 5, Tomcat 6

      Description

      Currently there are certain default actions implemented in CoreAdminHandler ( CREATE , SWAP, RELOAD , ALIAS etc.) .

      For the purpose of in-house monitoring tools that needs to interact with multiple cores at a given solr instance - we need custom handlers that has access to CoreContainer and the req, resp of the same.

      So - the proposed way of injecting handlers is as follows.

      In solr.xml - we add a new schema -

      <solr >
      <cores adminPath="/cores/admin">

      <adminActionHandler action="newaction" handlerType="com.mydomain.myclass" />
      </cores>
      </solr>

      New abstract class - CoreAdminActionRequestHandler added - that com.mydomain.myclass would need to inherit from.

      Following action handlers registered by default -

      registerCustomAdminHandler("create", new AdminCreateActionRequestHandler());
      registerCustomAdminHandler("rename", new AdminRenameActionRequestHandler());
      registerCustomAdminHandler("alias", new AdminAliasActionRequestHandler());
      registerCustomAdminHandler("unload", new AdminUnloadActionRequestHandler());
      registerCustomAdminHandler("status", new AdminStatusActionRequestHandler());
      registerCustomAdminHandler("persist", new AdminPersistActionRequestHandler());
      registerCustomAdminHandler("reload", new AdminReloadActionRequestHandler());
      registerCustomAdminHandler("swap", new AdminSwapActionRequestHandler());

      Trying to register a handler with one that already exists would result in an error ( Hence - the above mentioned defaults would not be overridden).

      1. SOLR-1106.patch
        5 kB
        Noble Paul
      2. SOLR-1106.patch
        20 kB
        Shalin Shekhar Mangar
      3. SOLR-1106.patch
        19 kB
        Karthik K
      4. SOLR-1106.patch
        7 kB
        Karthik K
      5. SOLR-1106.patch
        7 kB
        Karthik K
      6. SOLR-1106.patch
        29 kB
        Karthik K

        Issue Links

          Activity

          Hide
          Karthik K added a comment -

          Changes to CoreContainer ( inferring solr admin action handlers / loading them ).
          Changes to CoreAdminHandler ( registering handlers )
          New class - CoreAdminActionRequestHandler added ( that needs to be implemented by the plugins )

          Existing admin actions implemented as CoreAdminActionRequestHandlers.

          Show
          Karthik K added a comment - Changes to CoreContainer ( inferring solr admin action handlers / loading them ). Changes to CoreAdminHandler ( registering handlers ) New class - CoreAdminActionRequestHandler added ( that needs to be implemented by the plugins ) Existing admin actions implemented as CoreAdminActionRequestHandlers.
          Hide
          Karthik K added a comment - - edited

          Name of the actions is case-insensitive .

          Show
          Karthik K added a comment - - edited Name of the actions is case-insensitive .
          Hide
          Karthik K added a comment -

          Revised Patch with less drastic changes:

          • New method added to SolrResourceLoader# newAdminHandlerInstance(final CoreContainer coreContainer) to inject CoreContainer aware systems (ignoring SolrCoreAware since at a container level in a multi-core system - SolrCoreAware might not make sense)
          • CoreAdminHandler#handleCustomAction(final CoreContainer cores, final SolrQueryRequest req, final SolrQueryResponse rsp)
            added to handle custom actions ( By default - it throws an exception explaining that action is not valid ).
          • CoreContainer :
            solr/cores/@adminHandler added as an optional xml config parameter ( defaults to org.apache.solr.handler.admin.CoreAdminHandler ) as before.
          Show
          Karthik K added a comment - Revised Patch with less drastic changes: New method added to SolrResourceLoader# newAdminHandlerInstance(final CoreContainer coreContainer) to inject CoreContainer aware systems (ignoring SolrCoreAware since at a container level in a multi-core system - SolrCoreAware might not make sense) CoreAdminHandler#handleCustomAction(final CoreContainer cores, final SolrQueryRequest req, final SolrQueryResponse rsp) added to handle custom actions ( By default - it throws an exception explaining that action is not valid ). CoreContainer : solr/cores/@adminHandler added as an optional xml config parameter ( defaults to org.apache.solr.handler.admin.CoreAdminHandler ) as before.
          Hide
          Noble Paul added a comment -

          few observations

          • It leaves no option to override the standard commands. Move the execution of each command to separate protected methods. this way even the standard commands can be overridden
          • do not use reflection to load the default CoreAdminhandler
          Show
          Noble Paul added a comment - few observations It leaves no option to override the standard commands. Move the execution of each command to separate protected methods. this way even the standard commands can be overridden do not use reflection to load the default CoreAdminhandler
          Hide
          Karthik K added a comment -

          It leaves no option to override the standard commands.

          That seems counter-intuitive to your objection of the first design by Abstract classes and implementations since the correct way is to have an abstract class and provide default implementations of the same for commands available by default. Also - I do not see the motivation to override the default commands.

          I will attach the revised patch to load the default CoreAdminHandler without reflection.

          Show
          Karthik K added a comment - It leaves no option to override the standard commands. That seems counter-intuitive to your objection of the first design by Abstract classes and implementations since the correct way is to have an abstract class and provide default implementations of the same for commands available by default. Also - I do not see the motivation to override the default commands. I will attach the revised patch to load the default CoreAdminHandler without reflection.
          Hide
          Karthik K added a comment -

          CoreAdminHandler ( default ) instantiated without using reflection

          Show
          Karthik K added a comment - CoreAdminHandler ( default ) instantiated without using reflection
          Hide
          Noble Paul added a comment -

          I do not see the motivation to override the default commands.

          There are places were we may need to override the default implementation. Actually we already do it internally. So if I cannot override the the default commands it may not be as useful

          Show
          Noble Paul added a comment - I do not see the motivation to override the default commands. There are places were we may need to override the default implementation. Actually we already do it internally. So if I cannot override the the default commands it may not be as useful
          Hide
          Karthik K added a comment -

          There are places were we may need to override the default implementation. Actually we already do it internally. So if I cannot override the the default commands it may not be as useful

          If that were the case - then going by an abstract action and implementations of the same (with option to override default implementations ) would probably be cleaner , allowing default implementations to be overridden. Otherwise we are looking at probably around 6 - 7 methods that could be overridden and as we add more default commands - we might end up adding one more method to be overridden. Let me know what you feel about this.

          Show
          Karthik K added a comment - There are places were we may need to override the default implementation. Actually we already do it internally. So if I cannot override the the default commands it may not be as useful If that were the case - then going by an abstract action and implementations of the same (with option to override default implementations ) would probably be cleaner , allowing default implementations to be overridden. Otherwise we are looking at probably around 6 - 7 methods that could be overridden and as we add more default commands - we might end up adding one more method to be overridden. Let me know what you feel about this.
          Hide
          Noble Paul added a comment -

          to hoss' comment on the list http://markmail.org/message/s7mcbtaskngr74bd

          The commands such as create/load/unload etc can only be done by the CoreAdminHandler. So it is not really possible to achieve this as a RequestHandler. Take our usecase where we start with a blank slate ( zero cores) and we keep adding cores . In this case there is no core in the first place to attach a RequestHandler

          we might end up adding one more method to be overridden. Let me know what you feel about this.

          As I see it , there will be very few users overriding the CoreAdminHandler . We do it and we have a custom build of Solr for that. With this issue fixed I may be able to plugin my custom CoreAdminHandler. having 7 -8 methods to be overridden is a good idea. If there are new commands we may have new methods

          Show
          Noble Paul added a comment - to hoss' comment on the list http://markmail.org/message/s7mcbtaskngr74bd The commands such as create/load/unload etc can only be done by the CoreAdminHandler. So it is not really possible to achieve this as a RequestHandler. Take our usecase where we start with a blank slate ( zero cores) and we keep adding cores . In this case there is no core in the first place to attach a RequestHandler we might end up adding one more method to be overridden. Let me know what you feel about this. As I see it , there will be very few users overriding the CoreAdminHandler . We do it and we have a custom build of Solr for that. With this issue fixed I may be able to plugin my custom CoreAdminHandler. having 7 -8 methods to be overridden is a good idea. If there are new commands we may have new methods
          Hide
          Karthik K added a comment -

          Any Core can get access to the CoreContainer (via core.getCoreDescriptor().getCoreContainer()) and all of the SolrCores it is managing, so couldn't these new hooks you need be implented in regular RequestHandler?

          I ask this from the "how to achieve a niche goal with the minimal number of invasive changes" standpoint – mainly because i don't really understand what new types of "monitoring hooks" you're thinking of. if they seem like something that would be generally useful to lots of people, why not add them to CoreAdminHandler? if they pattern of adding them seems like something that will come up for lots of people then i would worry about making CoreAdminHandler more extensible.

          Monitoring hooks was one of the examples that we are using internally . If at the end of the implementation I find it generic enough - I would definitely be more than happy to share the patch for that too - but this I need this to be a building block on which other similar components could be built.

          Another context in which we need this is w.r.t doing some matching analysis between different indices (managed via - a multi-core instance) where the indices are managed by the respective SolrCore's UpdateHandlers while a req / response via http would goto a place that has access to both the cores and return the result,

          RequestHandler does not seem logically correct (though is indirectly possible via the method outlined earlier ) since that would bind the request Handler to one of the cores while the behavior intended is across cores - hence that needs to be done a level above all the cores and I selected admin Handler since that seems to be the only handler used in solr.xml .

          But what could be useful is to retain the requestHandler and make them configurable in solr.xml to address similar requests in the future.

          Show
          Karthik K added a comment - Any Core can get access to the CoreContainer (via core.getCoreDescriptor().getCoreContainer()) and all of the SolrCores it is managing, so couldn't these new hooks you need be implented in regular RequestHandler? I ask this from the "how to achieve a niche goal with the minimal number of invasive changes" standpoint – mainly because i don't really understand what new types of "monitoring hooks" you're thinking of. if they seem like something that would be generally useful to lots of people, why not add them to CoreAdminHandler? if they pattern of adding them seems like something that will come up for lots of people then i would worry about making CoreAdminHandler more extensible. Monitoring hooks was one of the examples that we are using internally . If at the end of the implementation I find it generic enough - I would definitely be more than happy to share the patch for that too - but this I need this to be a building block on which other similar components could be built. Another context in which we need this is w.r.t doing some matching analysis between different indices (managed via - a multi-core instance) where the indices are managed by the respective SolrCore's UpdateHandlers while a req / response via http would goto a place that has access to both the cores and return the result, RequestHandler does not seem logically correct (though is indirectly possible via the method outlined earlier ) since that would bind the request Handler to one of the cores while the behavior intended is across cores - hence that needs to be done a level above all the cores and I selected admin Handler since that seems to be the only handler used in solr.xml . But what could be useful is to retain the requestHandler and make them configurable in solr.xml to address similar requests in the future.
          Hide
          Karthik K added a comment - - edited
          As I see it , there will be very few users overriding the CoreAdminHandler . We do it and we have a custom build of Solr for that. With this issue fixed I may be able to plugin my custom CoreAdminHandler. having 7 -8 methods to be overridden is a good idea. If there are new commands we may have new methods

          I have to disagree with the suggestions of overriding 7 - 8 method as a good idea here. Also - it seems ironical to me that you have a commit privilege to solr and have a custom build too ( as opposed to a pluggable library to solr ) since given the right hooks available - it can prevent forking of the code and useful to others in the community as well.

          The point of my longer exercise of submitting a patch to get it committed is to prevent forking , even though it is tempting in terms of time to do it otherwise.

          Having put the reasons- here is the revised patch with individual 'actions' of CoreAdminHandler being implemented as separate methods ('protected') that could be overridden by derived methods if necesary.

          Show
          Karthik K added a comment - - edited As I see it , there will be very few users overriding the CoreAdminHandler . We do it and we have a custom build of Solr for that. With this issue fixed I may be able to plugin my custom CoreAdminHandler. having 7 -8 methods to be overridden is a good idea. If there are new commands we may have new methods I have to disagree with the suggestions of overriding 7 - 8 method as a good idea here. Also - it seems ironical to me that you have a commit privilege to solr and have a custom build too ( as opposed to a pluggable library to solr ) since given the right hooks available - it can prevent forking of the code and useful to others in the community as well. The point of my longer exercise of submitting a patch to get it committed is to prevent forking , even though it is tempting in terms of time to do it otherwise. Having put the reasons- here is the revised patch with individual 'actions' of CoreAdminHandler being implemented as separate methods ('protected') that could be overridden by derived methods if necesary.
          Hide
          Noble Paul added a comment -

          I have to disagree with the suggestions of overriding 7 - 8 method as a good idea here.

          the methods are protected does not mean we may have to override it. We only override a couple of functionalities.

          it seems ironical to me that you have a commit privilege to solr and have a custom build too

          We will be glad to contribute everything back to Solr trunk. We may even do it eventually. but, our use cases are very unique, and there may not be a need for such things for others. The changes are not limited to this. We needed a very lightweight Solr because we need to run close to 20000 cores per box.

          I am happy to see another user who has similar needs.

          Show
          Noble Paul added a comment - I have to disagree with the suggestions of overriding 7 - 8 method as a good idea here. the methods are protected does not mean we may have to override it. We only override a couple of functionalities. it seems ironical to me that you have a commit privilege to solr and have a custom build too We will be glad to contribute everything back to Solr trunk. We may even do it eventually. but, our use cases are very unique, and there may not be a need for such things for others. The changes are not limited to this. We needed a very lightweight Solr because we need to run close to 20000 cores per box. I am happy to see another user who has similar needs.
          Hide
          Karthik K added a comment -

          Let me know how this revised patch looks like and whether it fits your needs as well since I would prefer to have this committed and pick up a nightly build ( or a release , better) . Thanks for the help once again.

          Show
          Karthik K added a comment - Let me know how this revised patch looks like and whether it fits your needs as well since I would prefer to have this committed and pick up a nightly build ( or a release , better) . Thanks for the help once again.
          Hide
          Shalin Shekhar Mangar added a comment -

          Thanks Kay, this looks good.

          I've made the following changes over your last patch

          1. Re-formatted code to Lucene/Solr coding style. I know this is frowned upon for changes to existing code but CoreAdminHandler was the only place using a non-standard style of brace positioning and variable names.
          2. Made the CoreContainer instance protected
          3. Removed the CoreContainer parameter to all the action methods since it is an instance variable
          4. Removed the final keyword from variable/parameter definitions. That is just noise in this context, isn't it?

          I think this is ready to commit. I'll wait for a day for any feedback that others may have.

          Show
          Shalin Shekhar Mangar added a comment - Thanks Kay, this looks good. I've made the following changes over your last patch Re-formatted code to Lucene/Solr coding style. I know this is frowned upon for changes to existing code but CoreAdminHandler was the only place using a non-standard style of brace positioning and variable names. Made the CoreContainer instance protected Removed the CoreContainer parameter to all the action methods since it is an instance variable Removed the final keyword from variable/parameter definitions. That is just noise in this context, isn't it? I think this is ready to commit. I'll wait for a day for any feedback that others may have.
          Hide
          Karthik K added a comment -

          I have submitted changes to the wiki at - http://wiki.apache.org/solr/CoreAdmin with additional info about adminHandler introduced in this jira. Let me know how it looks and approve the same once this fix is in.

          Show
          Karthik K added a comment - I have submitted changes to the wiki at - http://wiki.apache.org/solr/CoreAdmin with additional info about adminHandler introduced in this jira. Let me know how it looks and approve the same once this fix is in.
          Hide
          Shalin Shekhar Mangar added a comment -

          Committed revision 768165.

          Thanks Kay and Noble!

          I have submitted changes to the wiki at - http://wiki.apache.org/solr/CoreAdmin with additional info about adminHandler introduced in this jira. Let me know how it looks and approve the same once this fix is in.

          This looks good Kay, thanks!

          Show
          Shalin Shekhar Mangar added a comment - Committed revision 768165. Thanks Kay and Noble! I have submitted changes to the wiki at - http://wiki.apache.org/solr/CoreAdmin with additional info about adminHandler introduced in this jira. Let me know how it looks and approve the same once this fix is in. This looks good Kay, thanks!
          Hide
          Karthik K added a comment -

          Thanks Shalin and Noble for helping committing this.

          Show
          Karthik K added a comment - Thanks Shalin and Noble for helping committing this.
          Hide
          Shalin Shekhar Mangar added a comment -

          It seems that the call to core.setProperties was removed in this refactoring and therefore, implicit properties are not being set on cores created through CoreAdminHandler.

          Show
          Shalin Shekhar Mangar added a comment - It seems that the call to core.setProperties was removed in this refactoring and therefore, implicit properties are not being set on cores created through CoreAdminHandler.
          Hide
          Shalin Shekhar Mangar added a comment -

          Oops, wrong call. That call was actually removed by SOLR-943.

          Show
          Shalin Shekhar Mangar added a comment - Oops, wrong call. That call was actually removed by SOLR-943 .
          Hide
          Noble Paul added a comment -

          The adminhandler attribute must be persisted back to solr.xml

          Show
          Noble Paul added a comment - The adminhandler attribute must be persisted back to solr.xml
          Hide
          Noble Paul added a comment -

          committed : 779439

          Show
          Noble Paul added a comment - committed : 779439
          Hide
          Grant Ingersoll added a comment -

          Bulk close for Solr 1.4

          Show
          Grant Ingersoll added a comment - Bulk close for Solr 1.4

            People

            • Assignee:
              Shalin Shekhar Mangar
              Reporter:
              Karthik K
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 3h
                3h
                Remaining:
                Remaining Estimate - 3h
                3h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development