Details

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

      Description

      A request handler with pluggable search components for things like:

      • standard
      • dismax
      • more-like-this
      • highlighting
      • field collapsing

      For more discussion, see:

      http://www.nabble.com/search-components-%28plugins%29-tf3898040.html#a11050274

      1. SOLR-281-SearchComponents.patch
        28 kB
        Ryan McKinley
      2. SOLR-281-SearchComponents.patch
        26 kB
        Sharad Agarwal
      3. SOLR-281-SearchComponents.patch
        62 kB
        Sharad Agarwal
      4. SOLR-281-SearchComponents.patch
        58 kB
        Sharad Agarwal
      5. SOLR-281-SearchComponents.patch
        69 kB
        Ryan McKinley
      6. SOLR-281-SearchComponents.patch
        70 kB
        Ryan McKinley
      7. SOLR-281-SearchComponents.patch
        60 kB
        Ryan McKinley
      8. SOLR-281-SearchComponents.patch
        59 kB
        Yonik Seeley
      9. SOLR-281-SearchComponents.patch
        62 kB
        Ryan McKinley
      10. SOLR-281-ComponentInit.patch
        27 kB
        Ryan McKinley
      11. SOLR-281-ComponentInit.patch
        28 kB
        Ryan McKinley
      12. solr-281.patch
        68 kB
        Henri Biestro
      13. solr-281.patch
        71 kB
        Henri Biestro
      14. solr-281.patch
        59 kB
        Yonik Seeley

        Issue Links

          Activity

          Hide
          Ryan McKinley added a comment -

          This is a quick - poorly documented / no testing - implementation that mimics the current Standard query.

          It is mostly intended to flush out design issues.

          This works by sticking a 'ResponseBuilder' in the context and sharing that between each component.

          Show
          Ryan McKinley added a comment - This is a quick - poorly documented / no testing - implementation that mimics the current Standard query. It is mostly intended to flush out design issues. This works by sticking a 'ResponseBuilder' in the context and sharing that between each component.
          Hide
          Pieter Berkel added a comment -

          I really like this modular approach to handling search requests, it will greatly simplify the process of adding new functionality (e.g. collapsing, faceting, more-like-this) to existing handlers without the need for unnecessary code replication. My primary goal is to extend the more-like-this handler capabilities and make them available to other handlers (such as dismax), and I think the proposed solution is a good approach.

          Some issues that I can forsee though are:

          1) Ordering: its fairly obvious that certain handlers need to be called before others (e.g. standard / dismax query parsing before faceting / highlighting) however there may be cases where the required sequence of events may be more subtle (e.g. faceting the results of a more-like-this query). There probably needs to be some mechanism to determine the order in which the components are prepared / processed.

          2) Dependancy: a situation may arise where a component depends on operations performed by another component (e.g. more-like-this may take advantage of the dismax 'bq' parameter), perhaps there needs to be some method of specifying component dependency so that the SearchHandler can load and process required components automatically?

          I hope this make sense, I'm fairly new to Solr development so I'm afraid my contributions to this issue would be mostly limited to (hopefully helpful) ideas and suggestions however I'm happy to tinker with the patched code from above and help test this new component framework as it is developed.

          cheers,
          Pieter

          Show
          Pieter Berkel added a comment - I really like this modular approach to handling search requests, it will greatly simplify the process of adding new functionality (e.g. collapsing, faceting, more-like-this) to existing handlers without the need for unnecessary code replication. My primary goal is to extend the more-like-this handler capabilities and make them available to other handlers (such as dismax), and I think the proposed solution is a good approach. Some issues that I can forsee though are: 1) Ordering: its fairly obvious that certain handlers need to be called before others (e.g. standard / dismax query parsing before faceting / highlighting) however there may be cases where the required sequence of events may be more subtle (e.g. faceting the results of a more-like-this query). There probably needs to be some mechanism to determine the order in which the components are prepared / processed. 2) Dependancy: a situation may arise where a component depends on operations performed by another component (e.g. more-like-this may take advantage of the dismax 'bq' parameter), perhaps there needs to be some method of specifying component dependency so that the SearchHandler can load and process required components automatically? I hope this make sense, I'm fairly new to Solr development so I'm afraid my contributions to this issue would be mostly limited to (hopefully helpful) ideas and suggestions however I'm happy to tinker with the patched code from above and help test this new component framework as it is developed. cheers, Pieter
          Hide
          Ryan McKinley added a comment -

          > Is anybody working on this issue?

          not actively.

          > if not, I can start looking at it.
          >

          Of course!

          Show
          Ryan McKinley added a comment - > Is anybody working on this issue? not actively. > if not, I can start looking at it. > Of course!
          Hide
          Sharad Agarwal added a comment -

          updatedprevious patch to work against trunk (r562043)

          Show
          Sharad Agarwal added a comment - updatedprevious patch to work against trunk (r562043)
          Hide
          Sharad Agarwal added a comment -

          Following changes from the previous patch:-
          -Supported components in DisMaxRequestHandler. Extended StandardRequestHandler and DisMaxRequestHandler from SearchHandler.
          -Deprecated StandardRequestHandler and DisMaxRequestHandler
          -Components configured via solrconfig.xml
          -Added DisMaxResponseBuilder, DisMaxQueryComponent and DisMaxDebugComponent classes

          Built and tested successfully with junit.

          Show
          Sharad Agarwal added a comment - Following changes from the previous patch:- -Supported components in DisMaxRequestHandler. Extended StandardRequestHandler and DisMaxRequestHandler from SearchHandler. -Deprecated StandardRequestHandler and DisMaxRequestHandler -Components configured via solrconfig.xml -Added DisMaxResponseBuilder, DisMaxQueryComponent and DisMaxDebugComponent classes Built and tested successfully with junit.
          Hide
          Pieter Berkel added a comment -

          I just tried this patch on svn trunk (r566899) and got the following failures:

          $ patch -p0 < ../SOLR-281-SearchComponents.patch
          ...
          patching file src/java/org/apache/solr/handler/StandardRequestHandler.java
          Hunk #1 succeeded at 17 with fuzz 1.
          Hunk #2 FAILED at 45.
          1 out of 2 hunks FAILED – saving rejects to file src/java/org/apache/solr/handler/StandardRequestHandler.java.rej
          ...
          patching file src/java/org/apache/solr/handler/DisMaxRequestHandler.java
          Hunk #1 FAILED at 17.
          1 out of 1 hunk FAILED – saving rejects to file src/java/org/apache/solr/handler/DisMaxRequestHandler.java.rej

          I suspect it is the changes made by SOLR-326 that is causing the these problems, would it be possible for you to create a new patch?

          thanks,
          Piete

          Show
          Pieter Berkel added a comment - I just tried this patch on svn trunk (r566899) and got the following failures: $ patch -p0 < ../ SOLR-281 -SearchComponents.patch ... patching file src/java/org/apache/solr/handler/StandardRequestHandler.java Hunk #1 succeeded at 17 with fuzz 1. Hunk #2 FAILED at 45. 1 out of 2 hunks FAILED – saving rejects to file src/java/org/apache/solr/handler/StandardRequestHandler.java.rej ... patching file src/java/org/apache/solr/handler/DisMaxRequestHandler.java Hunk #1 FAILED at 17. 1 out of 1 hunk FAILED – saving rejects to file src/java/org/apache/solr/handler/DisMaxRequestHandler.java.rej I suspect it is the changes made by SOLR-326 that is causing the these problems, would it be possible for you to create a new patch? thanks, Piete
          Hide
          Sharad Agarwal added a comment -

          Fixed the patch to work with the trunk.

          Show
          Sharad Agarwal added a comment - Fixed the patch to work with the trunk.
          Hide
          Ryan McKinley added a comment -

          Thanks Sharad! this is looking good.

          I just updated it to work with trunk and cleaned up a few things.

          • made SearchComponent an abstract class rather then an interface. This will make adding features in the future easier.
          • made SearchComponent implement SolrInfoMBean
          • added lots of ASF headers and comment stubs

          Yonik and/or Hoss... if you have time to check this out, that would be great.

          Show
          Ryan McKinley added a comment - Thanks Sharad! this is looking good. I just updated it to work with trunk and cleaned up a few things. made SearchComponent an abstract class rather then an interface. This will make adding features in the future easier. made SearchComponent implement SolrInfoMBean added lots of ASF headers and comment stubs Yonik and/or Hoss... if you have time to check this out, that would be great.
          Hide
          Pieter Berkel added a comment -

          I'm having trouble applying the latest patch to trunk (r575809) again:

          $ patch -p0 < ../SOLR-281-SearchComponents.patch
          ...
          patching file src/java/org/apache/solr/handler/StandardRequestHandler.java
          Hunk #1 FAILED at 17.
          Hunk #2 FAILED at 45.
          2 out of 2 hunks FAILED – saving rejects to file src/java/org/apache/solr/handler/StandardRequestHandler.java.rej
          patching file src/java/org/apache/solr/handler/DisMaxRequestHandler.java
          Hunk #2 FAILED at 118.
          1 out of 2 hunks FAILED – saving rejects to file src/java/org/apache/solr/handler/DisMaxRequestHandler.java.rej

          It also looks like the additions to solrconfig.xml have not been included in the latest patch either. I was also going to suggest that it might be a good idea to support class shorthand notation, so org.apache.solr.handler.component.* can be written solr.component.* in solrconfig.xml.

          Show
          Pieter Berkel added a comment - I'm having trouble applying the latest patch to trunk (r575809) again: $ patch -p0 < ../ SOLR-281 -SearchComponents.patch ... patching file src/java/org/apache/solr/handler/StandardRequestHandler.java Hunk #1 FAILED at 17. Hunk #2 FAILED at 45. 2 out of 2 hunks FAILED – saving rejects to file src/java/org/apache/solr/handler/StandardRequestHandler.java.rej patching file src/java/org/apache/solr/handler/DisMaxRequestHandler.java Hunk #2 FAILED at 118. 1 out of 2 hunks FAILED – saving rejects to file src/java/org/apache/solr/handler/DisMaxRequestHandler.java.rej It also looks like the additions to solrconfig.xml have not been included in the latest patch either. I was also going to suggest that it might be a good idea to support class shorthand notation, so org.apache.solr.handler.component.* can be written solr.component.* in solrconfig.xml.
          Hide
          Ryan McKinley added a comment -

          updated to work with trunk.

          Added the lost solrconfig.xml example:

          <requestHandler name="/search" class="solr.SearchHandler">
          <lst name="defaults">
          <str name="echoParams">explicit</str>
          </lst>
          <arr name="components">
          <str>org.apache.solr.handler.component.QueryComponent</str>
          <str>org.apache.solr.handler.component.FacetComponent</str>
          <str>org.apache.solr.handler.component.MoreLikeThisComponent</str>
          <str>org.apache.solr.handler.component.HighlightComponent</str>
          <str>org.apache.solr.handler.component.DebugComponent</str>
          </arr>
          </requestHandler>

          • - - -

          Re class aliases, it should work fine to put:
          solr.component.*
          If we add "org.apache.solr.handler.component." to the base list, you could just configure: org.apache.solr.handler.component.QueryComponent

          Show
          Ryan McKinley added a comment - updated to work with trunk. Added the lost solrconfig.xml example: <requestHandler name="/search" class="solr.SearchHandler"> <lst name="defaults"> <str name="echoParams">explicit</str> </lst> <arr name="components"> <str>org.apache.solr.handler.component.QueryComponent</str> <str>org.apache.solr.handler.component.FacetComponent</str> <str>org.apache.solr.handler.component.MoreLikeThisComponent</str> <str>org.apache.solr.handler.component.HighlightComponent</str> <str>org.apache.solr.handler.component.DebugComponent</str> </arr> </requestHandler> - - - Re class aliases, it should work fine to put: solr.component.* If we add "org.apache.solr.handler.component." to the base list, you could just configure: org.apache.solr.handler.component.QueryComponent
          Hide
          Henri Biestro added a comment -

          updated to work with trunk (587079);
          updated HighlightComponent to use SolrHighlighter.

          junit 0 failures/0 errors on Solaris 10 & WinXP.

          I've tried and failed to overcome the diff/patch rejects that Pieter experienced.
          I generated the patch from a Solaris 10 using GNU patch with:
          svn diff --diff-cmd /usr/bin/diff -x "-w -B -b -E -d -N -u" > ~/solr-281.patch
          I applied it to a clean trunk on Solaris 10 & WinXP (using cygwin) with:
          patch -u -p 0 < ~/solr-281.patch

          StandardRequestHandler.java generates rejects; replace the whole class definition with the '-' prefixed lines of the reject.

          WARNING since this is a 'unified' patch and not wanting to mess with Sharad & Ryan's patch versions, I attached the patch as solr-281.patch (not as SOLR-281-SearchComponents.patch)

          Show
          Henri Biestro added a comment - updated to work with trunk (587079); updated HighlightComponent to use SolrHighlighter. junit 0 failures/0 errors on Solaris 10 & WinXP. I've tried and failed to overcome the diff/patch rejects that Pieter experienced. I generated the patch from a Solaris 10 using GNU patch with: svn diff --diff-cmd /usr/bin/diff -x "-w -B -b -E -d -N -u" > ~/solr-281.patch I applied it to a clean trunk on Solaris 10 & WinXP (using cygwin) with: patch -u -p 0 < ~/solr-281.patch StandardRequestHandler.java generates rejects; replace the whole class definition with the '-' prefixed lines of the reject. WARNING since this is a 'unified' patch and not wanting to mess with Sharad & Ryan's patch versions, I attached the patch as solr-281.patch ( not as SOLR-281 -SearchComponents.patch)
          Hide
          Yonik Seeley added a comment -

          FYI, I'm in the process of updating this patch to work with trunk again (pluggable query parser stuff... SOLR-334)

          Show
          Yonik Seeley added a comment - FYI, I'm in the process of updating this patch to work with trunk again (pluggable query parser stuff... SOLR-334 )
          Hide
          Henri Biestro added a comment -

          Just a refresh on the latest trunk (587494) to keep things working before we got Yonik's proper version.
          junit 0 errors 0 failures.
          Same patch production process.

          Show
          Henri Biestro added a comment - Just a refresh on the latest trunk (587494) to keep things working before we got Yonik's proper version. junit 0 errors 0 failures. Same patch production process.
          Hide
          Yonik Seeley added a comment -

          Here's a new (smaller) patch that utilizes pluggable query parsers, and

          • removes DisMax specific modules since dismax specific logic is limited to query construction
          • DisMax request handler is now just the standard handler with defType=dismax added as a default
          • removed variable RequestBuilder class logic since it seems unnecessary... if two non-standard components want to communicate something, they can use the Context or the Response. (any reason I'm missing why it should stay?)

          Thoughts on these changes?

          We need to think through all the members of ResponseBuilder carefully and decide what component sets/reads them in what phase (and if that makes the most sense).

          Should ResponseBuilder have methods instead of members? If so, that would allow a component to perhaps even replace the ResponseBuilder and delegate to the original.

          How will a users custom component get configuration? Should components be a full plugins with an init() for configuration?

          Show
          Yonik Seeley added a comment - Here's a new (smaller) patch that utilizes pluggable query parsers, and removes DisMax specific modules since dismax specific logic is limited to query construction DisMax request handler is now just the standard handler with defType=dismax added as a default removed variable RequestBuilder class logic since it seems unnecessary... if two non-standard components want to communicate something, they can use the Context or the Response. (any reason I'm missing why it should stay?) Thoughts on these changes? We need to think through all the members of ResponseBuilder carefully and decide what component sets/reads them in what phase (and if that makes the most sense). Should ResponseBuilder have methods instead of members? If so, that would allow a component to perhaps even replace the ResponseBuilder and delegate to the original. How will a users custom component get configuration? Should components be a full plugins with an init() for configuration?
          Hide
          Ryan McKinley added a comment -

          Yonik, i could not get your last patch to apply cleanly to trunk. This is my best attempt to resolve the conflicts and fix a few things.

          1. remove the call to getResponseBuilder() in SearchComponent
          2. the timing info was commented out of debugging

          • - - - -

          The dismax changes look great - i like the new query parser stuff!

          > - removed variable RequestBuilder class logic since it seems unnecessary...
          > if two non-standard components want to communicate something, they can
          > use the Context or the Response. (any reason I'm missing why it should stay?)

          If at all possible, I think we should do our best to avoid depending on Map<String> for an interface.

          >
          > We need to think through all the members of ResponseBuilder carefully and decide what component sets/reads them in what phase (and if that makes the most sense).
          >

          yes

          >
          > Should ResponseBuilder have methods instead of members? If so, that would allow a component to perhaps even replace the ResponseBuilder and delegate to the original.
          >

          yes, that makes sense. How would a component replace the ResponseBuilder? If it could do that, there is obviously no need for the variable RequestBuilder class logic

          >
          > How will a users custom component get configuration? Should components be a full plugins with an init() for configuration?
          >

          As is, they are defined in a "components" section for SearchHandler (from the example solrconfig.xml):

          <requestHandler name="/search" class="solr.SearchHandler">
          <lst name="defaults">
          <str name="echoParams">explicit</str>
          </lst>
          <arr name="components">
          <str>org.apache.solr.handler.component.QueryComponent</str>
          <str>org.apache.solr.handler.component.FacetComponent</str>
          <str>org.apache.solr.handler.component.MoreLikeThisComponent</str>
          <str>org.apache.solr.handler.component.HighlightComponent</str>
          <str>org.apache.solr.handler.component.DebugComponent</str>
          </arr>
          </requestHandler>

          Perhaps the components should be passed the init params?

          Unless there is a compelling reason, I don't think components need to be shared across request handlers thus justifying a top level component registry.

          Show
          Ryan McKinley added a comment - Yonik, i could not get your last patch to apply cleanly to trunk. This is my best attempt to resolve the conflicts and fix a few things. 1. remove the call to getResponseBuilder() in SearchComponent 2. the timing info was commented out of debugging - - - - The dismax changes look great - i like the new query parser stuff! > - removed variable RequestBuilder class logic since it seems unnecessary... > if two non-standard components want to communicate something, they can > use the Context or the Response. (any reason I'm missing why it should stay?) If at all possible, I think we should do our best to avoid depending on Map<String> for an interface. > > We need to think through all the members of ResponseBuilder carefully and decide what component sets/reads them in what phase (and if that makes the most sense). > yes > > Should ResponseBuilder have methods instead of members? If so, that would allow a component to perhaps even replace the ResponseBuilder and delegate to the original. > yes, that makes sense. How would a component replace the ResponseBuilder? If it could do that, there is obviously no need for the variable RequestBuilder class logic > > How will a users custom component get configuration? Should components be a full plugins with an init() for configuration? > As is, they are defined in a "components" section for SearchHandler (from the example solrconfig.xml): <requestHandler name="/search" class="solr.SearchHandler"> <lst name="defaults"> <str name="echoParams">explicit</str> </lst> <arr name="components"> <str>org.apache.solr.handler.component.QueryComponent</str> <str>org.apache.solr.handler.component.FacetComponent</str> <str>org.apache.solr.handler.component.MoreLikeThisComponent</str> <str>org.apache.solr.handler.component.HighlightComponent</str> <str>org.apache.solr.handler.component.DebugComponent</str> </arr> </requestHandler> Perhaps the components should be passed the init params? Unless there is a compelling reason, I don't think components need to be shared across request handlers thus justifying a top level component registry.
          Hide
          Yonik Seeley added a comment -

          Yonik, i could not get your last patch to apply cleanly to trunk.

          That's OK, the one you produced fails for me on a clean checkout too.... looks like maybe we hit a little corner case with patch/diff.
          Should we perhaps commit this working version, marking ResponseBuilder as in-flux, and then continue generating patches from that???

          Show
          Yonik Seeley added a comment - Yonik, i could not get your last patch to apply cleanly to trunk. That's OK, the one you produced fails for me on a clean checkout too.... looks like maybe we hit a little corner case with patch/diff. Should we perhaps commit this working version, marking ResponseBuilder as in-flux, and then continue generating patches from that???
          Hide
          Yonik Seeley added a comment -

          I'll see if I can jiggle things a bit to get better patch behavior first...

          Show
          Yonik Seeley added a comment - I'll see if I can jiggle things a bit to get better patch behavior first...
          Hide
          Yonik Seeley added a comment -

          OK, uploading the one that works for me now.

          Show
          Yonik Seeley added a comment - OK, uploading the one that works for me now.
          Hide
          Yonik Seeley added a comment -

          Hold on... this patch is double in size from the last for some reason.... deleting.

          Show
          Yonik Seeley added a comment - Hold on... this patch is double in size from the last for some reason.... deleting.
          Hide
          Yonik Seeley added a comment -

          OK, this patch should do it... sorry for the noise.

          Show
          Yonik Seeley added a comment - OK, this patch should do it... sorry for the noise.
          Hide
          Ryan McKinley added a comment -

          ok, that one works (or close enough)

          >
          > Should we perhaps commit this working version, marking ResponseBuilder as in-flux, and then continue generating patches from that???
          >

          If you feel comfortable with the big picture, yes, I think we should commit this and refine the ResponseBuilder and perhaps the configuration options in smaller patches.

          Show
          Ryan McKinley added a comment - ok, that one works (or close enough) > > Should we perhaps commit this working version, marking ResponseBuilder as in-flux, and then continue generating patches from that??? > If you feel comfortable with the big picture, yes, I think we should commit this and refine the ResponseBuilder and perhaps the configuration options in smaller patches.
          Hide
          Ryan McKinley added a comment -

          Here is an updated version of your patch that converts the ResponseBuilder public variables to private vars with get/set methods.

          How do you feel about committing this and working out the ResponseBuilder details in subsequent smaller patches?

          Show
          Ryan McKinley added a comment - Here is an updated version of your patch that converts the ResponseBuilder public variables to private vars with get/set methods. How do you feel about committing this and working out the ResponseBuilder details in subsequent smaller patches?
          Hide
          Henri Biestro added a comment -

          Not sure process-wise whether votes/watchers got a say, my apologies if not; +1 otherwise.

          Show
          Henri Biestro added a comment - Not sure process-wise whether votes/watchers got a say, my apologies if not; +1 otherwise.
          Hide
          Yonik Seeley added a comment - - edited

          > How do you feel about committing this and working out the ResponseBuilder details in subsequent smaller patches?

          +1

          In this case I think the current patch represents the right direction, seems to work fine, and committing now will save effort of people working on other patches.

          Show
          Yonik Seeley added a comment - - edited > How do you feel about committing this and working out the ResponseBuilder details in subsequent smaller patches? +1 In this case I think the current patch represents the right direction, seems to work fine, and committing now will save effort of people working on other patches.
          Hide
          Ryan McKinley added a comment -

          The core changes are committed in rev594268.

          I will resolve this issue and open a new issue to track cleaning up the ResponseBuilder

          Show
          Ryan McKinley added a comment - The core changes are committed in rev594268. I will resolve this issue and open a new issue to track cleaning up the ResponseBuilder
          Hide
          Ryan McKinley added a comment -

          Lets track ResponseBuilder issues with SOLR-410

          Show
          Ryan McKinley added a comment - Lets track ResponseBuilder issues with SOLR-410
          Hide
          Ryan McKinley added a comment -

          The SearchComponent framework needs to allow for custom component initialization – I previously believed this could be done from within the RequestHandler NamedList args. In trying to use this for a custom component that needs to load a resource file, I found it is woefully inadequate.

          As Yonik suggested a while back (and I disagreed with), I now think components should be loaded and initialized the same way as RequestHandlers. That is, a top level name based registry in SolrCore that SearchHandlers can share. For example, solrconfig.xml could declare:

             <searchComponent name="query"     class="org.apache.solr.handler.component.QueryComponent" />
             <searchComponent name="facet"     class="org.apache.solr.handler.component.FacetComponent" />
             <searchComponent name="mlt"       class="org.apache.solr.handler.component.MoreLikeThisComponent" />
             <searchComponent name="highlight" class="org.apache.solr.handler.component.HighlightComponent" />
             <searchComponent name="debug"     class="org.apache.solr.handler.component.DebugComponent" />
          

          and a SearchHandler can pick and choose what components are used with:

           <requestHandler name="/search" class="solr.SearchHandler">
              <arr name="components">
                <str>query</str>
                <str>facet</str>
                <str>mlt</str>
                <str>highlight</str>
                <str>debug</str>
              </arr>
            </requestHandler>
          

          Also, there should be a way to configure components to run before or after the standard component list without having to know and maintain what the "standard" list is. For example, when we add field collapsing to the 'standard' options, it should not require everyone to update their solrconfig.xml

          When SOLR-414 is committed, I will attach a patch using this strategy.

          Show
          Ryan McKinley added a comment - The SearchComponent framework needs to allow for custom component initialization – I previously believed this could be done from within the RequestHandler NamedList args. In trying to use this for a custom component that needs to load a resource file, I found it is woefully inadequate. As Yonik suggested a while back (and I disagreed with), I now think components should be loaded and initialized the same way as RequestHandlers. That is, a top level name based registry in SolrCore that SearchHandlers can share. For example, solrconfig.xml could declare: <searchComponent name= "query" class= "org.apache.solr.handler.component.QueryComponent" /> <searchComponent name= "facet" class= "org.apache.solr.handler.component.FacetComponent" /> <searchComponent name= "mlt" class= "org.apache.solr.handler.component.MoreLikeThisComponent" /> <searchComponent name= "highlight" class= "org.apache.solr.handler.component.HighlightComponent" /> <searchComponent name= "debug" class= "org.apache.solr.handler.component.DebugComponent" /> and a SearchHandler can pick and choose what components are used with: <requestHandler name= "/search" class= "solr.SearchHandler" > <arr name= "components" > <str> query </str> <str> facet </str> <str> mlt </str> <str> highlight </str> <str> debug </str> </arr> </requestHandler> Also, there should be a way to configure components to run before or after the standard component list without having to know and maintain what the "standard" list is. For example, when we add field collapsing to the 'standard' options, it should not require everyone to update their solrconfig.xml When SOLR-414 is committed, I will attach a patch using this strategy.
          Hide
          Ryan McKinley added a comment -

          This patch uses SOLR-414 Initialization strategy for SearchComponents.

          This makes SearchComponents a top level plugin (just like RequestHandlers) that get registered to a unique name in SolrCore. A SearchHandler can configure the component chain using:

            <requestHandler name="/search" class="solr.SearchHandler">
              <lst name="defaults">
                <str name="echoParams">explicit</str>
              </lst>
              <!--
              By default, this will register the following components:
              
              <arr name="components">
                <str>query</str>
                <str>facet</str>
                <str>mlt</str>
                <str>highlight</str>
                <str>debug</str>
              </arr>
              
              To insert handlers before or after the 'standard' components, use:
              
              <arr name="first-components">
                <str>first</str>
              </arr>
              
              <arr name="last-components">
                <str>last</str>
              </arr>
              
              -->
            </requestHandler>
          
          Show
          Ryan McKinley added a comment - This patch uses SOLR-414 Initialization strategy for SearchComponents. This makes SearchComponents a top level plugin (just like RequestHandlers) that get registered to a unique name in SolrCore. A SearchHandler can configure the component chain using: <requestHandler name= "/search" class= "solr.SearchHandler" > <lst name= "defaults" > <str name= "echoParams" > explicit </str> </lst> <!-- By default, this will register the following components: <arr name= "components" > <str> query </str> <str> facet </str> <str> mlt </str> <str> highlight </str> <str> debug </str> </arr> To insert handlers before or after the 'standard' components, use: <arr name= "first-components" > <str> first </str> </arr> <arr name= "last-components" > <str> last </str> </arr> --> </requestHandler>
          Hide
          Yonik Seeley added a comment -

          The other idea I had was numeric positioning... query=1000, facet=2000, etc, and so a user could add their component at any point. Perhaps your first/last is sufficient though.

          Show
          Yonik Seeley added a comment - The other idea I had was numeric positioning... query=1000, facet=2000, etc, and so a user could add their component at any point. Perhaps your first/last is sufficient though.
          Hide
          Ryan McKinley added a comment -

          The problem first/last is trying to solve is to let a custom handler automatically incorporate default changes to the standard components without editing solrconfig.xml.

          Numeric positioning seems a bit brittle unless the 'standard' components have a locked position. Without opening the door to legacy problems, I don't see any good way to insert a component between "facet" and "mlt" without specifying the whole chain.

          Show
          Ryan McKinley added a comment - The problem first/last is trying to solve is to let a custom handler automatically incorporate default changes to the standard components without editing solrconfig.xml. Numeric positioning seems a bit brittle unless the 'standard' components have a locked position. Without opening the door to legacy problems, I don't see any good way to insert a component between "facet" and "mlt" without specifying the whole chain.
          Hide
          Ryan McKinley added a comment -

          updated initialization patch from SOLR-418. Without objection, I will commit this soon.

          Show
          Ryan McKinley added a comment - updated initialization patch from SOLR-418 . Without objection, I will commit this soon.
          Hide
          Sabyasachi Dalal added a comment -

          I am updating the distributed search patch (SOLR-303) with this patch.
          I added the dist search components as,

          <searchComponent name="gstat" class="org.apache.solr.handler.federated.component.GlobalCollectionStatComponent" />
          <searchComponent name="mqp" class="org.apache.solr.handler.federated.component.MainQPhaseComponent" />
          <searchComponent name="aqp" class="org.apache.solr.handler.federated.component.AuxiliaryQPhaseComponent" />

          <requestHandler name="/search" class="solr.SearchHandler">
          <lst name="defaults">
          <str name="echoParams">explicit</str>
          </lst>

          <arr name="last-components">
          <str>gstat</str>
          <str>mqp</str>
          <str>aqp</str>
          </arr>

          </requestHandler>

          But it was not working. On debugging i found that these added components were not getting registered.

          I made the following change in SolrCore.loadSearchComponents,

          // NamedListPluginLoader<SearchComponent> loader = new NamedListPluginLoader<SearchComponent>( xpath, searchComponents );
          NamedListPluginLoader<SearchComponent> loader = new NamedListPluginLoader<SearchComponent>( xpath, components );

          Show
          Sabyasachi Dalal added a comment - I am updating the distributed search patch ( SOLR-303 ) with this patch. I added the dist search components as, <searchComponent name="gstat" class="org.apache.solr.handler.federated.component.GlobalCollectionStatComponent" /> <searchComponent name="mqp" class="org.apache.solr.handler.federated.component.MainQPhaseComponent" /> <searchComponent name="aqp" class="org.apache.solr.handler.federated.component.AuxiliaryQPhaseComponent" /> <requestHandler name="/search" class="solr.SearchHandler"> <lst name="defaults"> <str name="echoParams">explicit</str> </lst> <arr name="last-components"> <str>gstat</str> <str>mqp</str> <str>aqp</str> </arr> </requestHandler> But it was not working. On debugging i found that these added components were not getting registered. I made the following change in SolrCore.loadSearchComponents, // NamedListPluginLoader<SearchComponent> loader = new NamedListPluginLoader<SearchComponent>( xpath, searchComponents ); NamedListPluginLoader<SearchComponent> loader = new NamedListPluginLoader<SearchComponent>( xpath, components );
          Hide
          Ryan McKinley added a comment -

          Sabyasachi - check that your patch does not comment out the loading line...

          the committed version of this patch is not commented out:
          http://svn.apache.org/repos/asf/lucene/solr/trunk/src/java/org/apache/solr/core/SolrCore.java

          Show
          Ryan McKinley added a comment - Sabyasachi - check that your patch does not comment out the loading line... the committed version of this patch is not commented out: http://svn.apache.org/repos/asf/lucene/solr/trunk/src/java/org/apache/solr/core/SolrCore.java
          Hide
          Michael Dodsworth added a comment -

          This is great; decomposing the handler and allowing the components to be wired up in the config really helps development (and maintenance of those changes).

          For my purposes, I needed to make a change to the way the dismax query was being generated. Using the DisMaxQParserPlugin as a template, I created my own QParser and associated QParserPlugin; changed the relevant bits; added a <queryParser...> entry in solrconfig.xml; added the 'defType' parameter to the wanted SearchHandler configuration...and...all works well.

          Just a few comments:

          • I had to make the QParser parse() method public (as the new query parser may still need to use the existing query parsers (backup lucene parser, boost parser, function parser, etc).
          • The QParserPlugin class seems unnecessary: all it does is implement init() and add a createParser method. Why not just have the parser constructor take those arguments...or, if that can't be done, create an interface to allow the parser itself implement both init() and createParser() (or create()). It then avoids having to create 2 classes (in the case of DisMax, in the same file...which is not pretty).
          Show
          Michael Dodsworth added a comment - This is great; decomposing the handler and allowing the components to be wired up in the config really helps development (and maintenance of those changes). For my purposes, I needed to make a change to the way the dismax query was being generated. Using the DisMaxQParserPlugin as a template, I created my own QParser and associated QParserPlugin; changed the relevant bits; added a <queryParser...> entry in solrconfig.xml; added the 'defType' parameter to the wanted SearchHandler configuration...and...all works well. Just a few comments: I had to make the QParser parse() method public (as the new query parser may still need to use the existing query parsers (backup lucene parser, boost parser, function parser, etc). The QParserPlugin class seems unnecessary: all it does is implement init() and add a createParser method. Why not just have the parser constructor take those arguments...or, if that can't be done, create an interface to allow the parser itself implement both init() and createParser() (or create()). It then avoids having to create 2 classes (in the case of DisMax, in the same file...which is not pretty).
          Hide
          Yonik Seeley added a comment -

          The QParserPlugin class seems unnecessary: all it does is implement init() and add a createParser method. Why not just have the parser constructor take those arguments...

          That would require instantiation with reflection I think.

          or, if that can't be done, create an interface to allow the parser itself implement both init() and createParser() (or create()). It then avoids having to create 2 classes (in the case of DisMax, in the same file...which is not pretty).

          QParserPlugin is that interface essentially (except that its an class instead of an interface). For library maintainers an abstract class is preferred over an interface for things that a user will extend... that way signature changes can be made in a backward compatible manner.

          Show
          Yonik Seeley added a comment - The QParserPlugin class seems unnecessary: all it does is implement init() and add a createParser method. Why not just have the parser constructor take those arguments... That would require instantiation with reflection I think. or, if that can't be done, create an interface to allow the parser itself implement both init() and createParser() (or create()). It then avoids having to create 2 classes (in the case of DisMax, in the same file...which is not pretty). QParserPlugin is that interface essentially (except that its an class instead of an interface). For library maintainers an abstract class is preferred over an interface for things that a user will extend... that way signature changes can be made in a backward compatible manner.
          Hide
          Michael Dodsworth added a comment -

          That would require instantiation with reflection I think.

          Reflection is already being used to create the QParserPlugins (SolrCore:1027 and AbstractPluginLoader:83) - I'm guessing the reason for the plugin is just to avoid creating instances through reflection on every parse (as you could keep hold of the QParser class and call newInstance). The second point is moot, once you take away the need for createParser(...).

          It's really not that big-a-deal, in the scheme of things.

          QParserPlugin is that interface essentially (except that its an class instead of an interface). For library maintainers an abstract class is preferred over an interface for things that a user will extend... that way signature changes can be made in a backward compatible manner.

          As an aside, method signature changes are usually trivial to fix; personally, the pain of those fixes is favourable to extending an abstract class unnecessarily.
          Are there any architectural reworking projects on the roadmap? I'm sure backward compatibility is a massive concern; perhaps with the more modular plugin design route Solr is going down, those concerns can be addressed. If there's a chance of being accepted, I would love to contribute a move towards using Spring.

          Show
          Michael Dodsworth added a comment - That would require instantiation with reflection I think. Reflection is already being used to create the QParserPlugins (SolrCore:1027 and AbstractPluginLoader:83) - I'm guessing the reason for the plugin is just to avoid creating instances through reflection on every parse (as you could keep hold of the QParser class and call newInstance). The second point is moot, once you take away the need for createParser(...). It's really not that big-a-deal, in the scheme of things. QParserPlugin is that interface essentially (except that its an class instead of an interface). For library maintainers an abstract class is preferred over an interface for things that a user will extend... that way signature changes can be made in a backward compatible manner. As an aside, method signature changes are usually trivial to fix; personally, the pain of those fixes is favourable to extending an abstract class unnecessarily. Are there any architectural reworking projects on the roadmap? I'm sure backward compatibility is a massive concern; perhaps with the more modular plugin design route Solr is going down, those concerns can be addressed. If there's a chance of being accepted, I would love to contribute a move towards using Spring.
          Hide
          Yonik Seeley added a comment -
          Show
          Yonik Seeley added a comment - Followed up on solr-dev to avoid stealing more of this JIRA isse: http://www.nabble.com/Re%3A--jira--Commented%3A-Search-Components-%28plugins%29-to15227648.html#a15227648
          Hide
          Ryan McKinley added a comment -

          I'll mark this issue as "fixed" – I'm not sure why it was still open, the reason it was repoened has been resolved. Any new issues should get their own JIRA issue

          Show
          Ryan McKinley added a comment - I'll mark this issue as "fixed" – I'm not sure why it was still open, the reason it was repoened has been resolved. Any new issues should get their own JIRA issue

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development