Solr
  1. Solr
  2. SOLR-16

patch to allow the use of a custom query output writer

    Details

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

      Description

      This patch allows users to create their own implementation of QueryResponseWriter, and specify that class in solrconfig.xml. The default behavior is to use XMLResponseWriter like before. The class selection code only runs once at servlet init time, so it shouldn't have any measurable effect on performance.

      1. custom_writer_rev2.patch
        6 kB
        Mike Baranczak
      2. custom_writer_rev3.patch
        10 kB
        Mike Baranczak
      3. custom_writer.patch
        3 kB
        Mike Baranczak
      4. OutputWriterTest.java
        2 kB
        Mike Baranczak

        Activity

        Hide
        Erik Hatcher added a comment -

        I would like to see a request time toggle on the writer used, just like the request handler gets decided. I would like the client to control the format returned. That shouldn't be a big refactoring - would you mind implementing that instead?

        Show
        Erik Hatcher added a comment - I would like to see a request time toggle on the writer used, just like the request handler gets decided. I would like the client to control the format returned. That shouldn't be a big refactoring - would you mind implementing that instead?
        Hide
        Yonik Seeley added a comment -

        > I would like to see a request time toggle on the writer used, just like the request handler gets decided.

        +1
        The default could be configured per request-handler and overridden by specific requests.

        Show
        Yonik Seeley added a comment - > I would like to see a request time toggle on the writer used, just like the request handler gets decided. +1 The default could be configured per request-handler and overridden by specific requests.
        Hide
        Mike Baranczak added a comment -

        > I would like to see a request time toggle on the writer used, just like the request handler gets decided.

        Using the same selection logic as for the request handlers does make more sense. Shouldn't be too hard to do.

        > The default could be configured per request-handler and overridden by specific requests.

        Right now, the request handler doesn't touch the output writer (the writer is called from SolrServlet) - so trying to associate the writer and the handler could get a wee bit messy. I'd much rather just keep those two aspects of the configuration separate. Simpler for me, and simpler for the people configuring Solr.

        Show
        Mike Baranczak added a comment - > I would like to see a request time toggle on the writer used, just like the request handler gets decided. Using the same selection logic as for the request handlers does make more sense. Shouldn't be too hard to do. > The default could be configured per request-handler and overridden by specific requests. Right now, the request handler doesn't touch the output writer (the writer is called from SolrServlet) - so trying to associate the writer and the handler could get a wee bit messy. I'd much rather just keep those two aspects of the configuration separate. Simpler for me, and simpler for the people configuring Solr.
        Hide
        Erik Hatcher added a comment -

        I'm fine with this configurability going in as is, and then we morph it to allow a request parameter to select the writer as well, of course using the configured default so no client has to change or specify a writer unless they want something special (like Ruby code output rather than XML

        Show
        Erik Hatcher added a comment - I'm fine with this configurability going in as is, and then we morph it to allow a request parameter to select the writer as well, of course using the configured default so no client has to change or specify a writer unless they want something special (like Ruby code output rather than XML
        Hide
        Mike Baranczak added a comment -

        Revised patch enclosed.

        Multiple writers may be specified in solrconfig.xml. The syntax is pretty much the same as for specifying rewuest handlers; see example/solr/conf/solrconfig.xml for example. Writers may be selected by adding the parameter "wt" (writer type) to the request. XMLResponseWriter is the default.

        Show
        Mike Baranczak added a comment - Revised patch enclosed. Multiple writers may be specified in solrconfig.xml. The syntax is pretty much the same as for specifying rewuest handlers; see example/solr/conf/solrconfig.xml for example. Writers may be selected by adding the parameter "wt" (writer type) to the request. XMLResponseWriter is the default.
        Hide
        Hoss Man added a comment -

        I just read through the patch – didn't have a chance to try it out yet – and both the functionality and the implimentation look great!

        Can i ask for one smal favor before it gets applied: some test cases showing requests work with and with out the param specified when there are multiple options in the config?

        leaving src/test/test-files/solr/conf/solrconfig.xml as is will test the default situation where no writers are lsted ... you can modify src/test/test-files/solr/crazy-path-to-config.xml to include both the default and some new writer and then clone the SampleTest and add some test methods to check that you get the output you expected from various writters.

        You don't even need to provide a usefull writer ... even something that just outputs some static text would work, just put it in src/test so people know it's not a "production" writer.

        most of the helper methods in AbstractSolrTestCase try to validate that the responses are wellformed XML, but if you want to test a writer that isn't XML, it can still be done using the TerstHarness directly ... BasicFunctionalityTest.testMultipleUpdatesPerAdd shows an example.

        Show
        Hoss Man added a comment - I just read through the patch – didn't have a chance to try it out yet – and both the functionality and the implimentation look great! Can i ask for one smal favor before it gets applied: some test cases showing requests work with and with out the param specified when there are multiple options in the config? leaving src/test/test-files/solr/conf/solrconfig.xml as is will test the default situation where no writers are lsted ... you can modify src/test/test-files/solr/crazy-path-to-config.xml to include both the default and some new writer and then clone the SampleTest and add some test methods to check that you get the output you expected from various writters. You don't even need to provide a usefull writer ... even something that just outputs some static text would work, just put it in src/test so people know it's not a "production" writer. most of the helper methods in AbstractSolrTestCase try to validate that the responses are wellformed XML, but if you want to test a writer that isn't XML, it can still be done using the TerstHarness directly ... BasicFunctionalityTest.testMultipleUpdatesPerAdd shows an example.
        Hide
        Mike Baranczak added a comment -

        Yeah, I can write a test. I can't do it right now, but I'll probably get to it sometime early next week.

        Show
        Mike Baranczak added a comment - Yeah, I can write a test. I can't do it right now, but I'll probably get to it sometime early next week.
        Hide
        Mike Baranczak added a comment -

        OK, I tried writing a test case like Hoss suggested and it kept failing when I tried to select a custom writer. I eventually figured out that this is because TestHarness is hard-coded to use XMLResponseWriter. Which made perfect sense with the old set-up, but now...

        I could patch TestHarness so that it honors the queryResponseWriter settings, but this wouldn't make a very useful test, since it would completely bypass the code that actually does the work of selecting the writer (in SolrServlet). And if we actually wanted to test SolrServlet, we'd have to completely rewrite the test framework.

        Any thoughts?

        Show
        Mike Baranczak added a comment - OK, I tried writing a test case like Hoss suggested and it kept failing when I tried to select a custom writer. I eventually figured out that this is because TestHarness is hard-coded to use XMLResponseWriter. Which made perfect sense with the old set-up, but now... I could patch TestHarness so that it honors the queryResponseWriter settings, but this wouldn't make a very useful test, since it would completely bypass the code that actually does the work of selecting the writer (in SolrServlet). And if we actually wanted to test SolrServlet, we'd have to completely rewrite the test framework. Any thoughts?
        Hide
        Hoss Man added a comment -

        I think the right solution is to refactor the writer selection logic you added to the SolrServlet into a method in SolrCore...

        public QueryResponseWriter getQueryResponseWriter(String handlerName)

        ...i would also suggest adding another new method that calls that one...

        public QueryResponseWriter getQueryResponseWriter(SolrQueryRequest)

        ...and put the query param checking and default writer fallback logic in there.

        Then change SolrServlet and the TestHarness to call that.

        How does that sound?

        (One other small thing now that I'm looking at the patch a little more closely: when handling exceptions, SolrException.logOnce is probably a better choice then ex.printStackTrace() – that way the logging information goes wherever the ServletContainer has it's logging configured to instead of just STDERR)

        Show
        Hoss Man added a comment - I think the right solution is to refactor the writer selection logic you added to the SolrServlet into a method in SolrCore... public QueryResponseWriter getQueryResponseWriter(String handlerName) ...i would also suggest adding another new method that calls that one... public QueryResponseWriter getQueryResponseWriter(SolrQueryRequest) ...and put the query param checking and default writer fallback logic in there. Then change SolrServlet and the TestHarness to call that. How does that sound? (One other small thing now that I'm looking at the patch a little more closely: when handling exceptions, SolrException.logOnce is probably a better choice then ex.printStackTrace() – that way the logging information goes wherever the ServletContainer has it's logging configured to instead of just STDERR)
        Hide
        Mike Baranczak added a comment -

        I've incorporated all of of Hoss' suggestions. All the units tests are passing, and everything else looks good.

        I do like the idea of passing init parameters to the writer, but I unfortunately don't have time to work on it right now.

        (custom_writer_rev3.patch and OutputWriterTest.java should be applied together - disregard all other files attached to this issue.)

        Show
        Mike Baranczak added a comment - I've incorporated all of of Hoss' suggestions. All the units tests are passing, and everything else looks good. I do like the idea of passing init parameters to the writer, but I unfortunately don't have time to work on it right now. (custom_writer_rev3.patch and OutputWriterTest.java should be applied together - disregard all other files attached to this issue.)
        Hide
        Hoss Man added a comment -

        patch looks very good .. i'm making a few tweaks and then I'll apply.

        Show
        Hoss Man added a comment - patch looks very good .. i'm making a few tweaks and then I'll apply.
        Hide
        Hoss Man added a comment -

        I did a little refactoring in initWriters to take advantage of some existing utility methods and commited this.

        Thanks for the patch Mike.

        Show
        Hoss Man added a comment - I did a little refactoring in initWriters to take advantage of some existing utility methods and commited this. Thanks for the patch Mike.
        Hide
        Erik Hatcher added a comment -

        Mike - thank you for your patience in refactoring what was already working for you into something more generalizable than you needed for the benefit of the community. I personally will be experimenting with this new feature.

        Thank you Hoss for applying the patches and following through on making this fit the current Solr architecture.

        Open source development at its finest!

        Show
        Erik Hatcher added a comment - Mike - thank you for your patience in refactoring what was already working for you into something more generalizable than you needed for the benefit of the community. I personally will be experimenting with this new feature. Thank you Hoss for applying the patches and following through on making this fit the current Solr architecture. Open source development at its finest!
        Hide
        Hoss Man added a comment -

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

        • Marked ("Resolved" or "Closed") and "Fixed"
        • Had no "Fix Version" versions
        • Was listed in the CHANGES.txt for 1.1

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

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

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

          People

          • Assignee:
            Hoss Man
            Reporter:
            Mike Baranczak
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development