Solr
  1. Solr
  2. SOLR-59

Copy request parameters to Solr's response

    Details

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

      Description

      This patch copies the request parameters (explicit ones only, not the defaults) to Solr's XML output.

      It is not configurable yet, it is enabled by default and adds a "queryParameters" list to the responseHeader:

      <responseHeader>
      <status>0</status>
      <QTime>1</QTime>
      <lst name="queryParameters">
      <arr name="multi">
      <str>red</str>
      <str>blue</str>
      </arr>
      <str name="rows">10</str>
      <str name="start">0</str>
      <str name="indent">on</str>
      <str name="q">solr</str>
      <str name="stylesheet"/>
      <str name="version">2.1</str>
      </lst>
      </responseHeader>

      The above example includes a multi-valued parameter, "multi".

      This might still change a bit, but if someone wants to play with it or improve it, here you go.

      1. SOLR-59-20061024.patch
        5 kB
        Bertrand Delacretaz
      2. SOLR-59-20061102.patch
        8 kB
        Bertrand Delacretaz
      3. SOLR-59-20061103.patch
        8 kB
        Bertrand Delacretaz
      4. SOLR-59-20061106.patch
        9 kB
        Bertrand Delacretaz
      5. SOLR-59-20061106-newfiles.tar.gz
        2 kB
        Bertrand Delacretaz
      6. SOLR-59-20061213.patch
        25 kB
        Yonik Seeley
      7. SOLR-59-new-files-20061102.tar.gz
        2 kB
        Bertrand Delacretaz

        Issue Links

          Activity

          Hide
          Yonik Seeley added a comment -

          I haven't had a chance to check the code, but I like the format.
          Personally, I think the default should be off, but if the standard SolrParams.getParam() is used to control the behavior, people may set the default to whatever they like.

          Show
          Yonik Seeley added a comment - I haven't had a chance to check the code, but I like the format. Personally, I think the default should be off, but if the standard SolrParams.getParam() is used to control the behavior, people may set the default to whatever they like.
          Hide
          Hoss Man added a comment -

          Some assorted comments on the patch...

          Independent of wether we need this functionality or not, i like the addition of the "public Iterator<String> getParameterNames()" to the SolrParams API ... but I think there is a bug in the DefaultSolrParams version: "defaults" is never consulted for param names.

          As I mentioned on the list, I think we should only do this for the orriginal params as specified in the URL (or POST body) ... in which case req.getOriginalParams() would be used instead of req.getParams().

          This patch also only modifies XMLWriter, making it a QueryResponseWriter specific piece of functionality ... it hadn't really occured to be when discussing this, but that seems like a good place for the code to live and fits with the idea of putting this info in the responseHeaer – which is output format specific anyway.

          There also seems to be some concensus on the list that there should be a param contrlling wether or not this logic is active, with the default being "false"

          Show
          Hoss Man added a comment - Some assorted comments on the patch... Independent of wether we need this functionality or not, i like the addition of the "public Iterator<String> getParameterNames()" to the SolrParams API ... but I think there is a bug in the DefaultSolrParams version: "defaults" is never consulted for param names. As I mentioned on the list, I think we should only do this for the orriginal params as specified in the URL (or POST body) ... in which case req.getOriginalParams() would be used instead of req.getParams(). This patch also only modifies XMLWriter, making it a QueryResponseWriter specific piece of functionality ... it hadn't really occured to be when discussing this, but that seems like a good place for the code to live and fits with the idea of putting this info in the responseHeaer – which is output format specific anyway. There also seems to be some concensus on the list that there should be a param contrlling wether or not this logic is active, with the default being "false"
          Hide
          Bertrand Delacretaz added a comment -

          Coming back to this according to the mailing list discussions (http://marc.theaimsgroup.com/?t=116167803100001&r=1&w=2), here's a slightly modified plan:

          1. Add a "headers" NamedList to SolrQueryResponse, to hold values that are meant for the responseHeader, makes these values available to all QueryResponseWriters.

          2. Have SolrCore put the computed "qtime" value in there (along with other stats if needed) instead of computing it in the QueryResponseWriter.

          3. In SolrCore.execute(), call a method to copy (if so configured) the explicit request parameters to a nested NamedList stored in the above "headers" NamedList.

          If people need more than the explicit parameters, this would be the place to change later on, but right now this will be explicit params only.

          4. In QueryResponseWriters, implement generic code to copy the "headers" NamedList to the output

          I think this puts everything in the right place - comments are welcome of course.

          Show
          Bertrand Delacretaz added a comment - Coming back to this according to the mailing list discussions ( http://marc.theaimsgroup.com/?t=116167803100001&r=1&w=2 ), here's a slightly modified plan: 1. Add a "headers" NamedList to SolrQueryResponse, to hold values that are meant for the responseHeader, makes these values available to all QueryResponseWriters. 2. Have SolrCore put the computed "qtime" value in there (along with other stats if needed) instead of computing it in the QueryResponseWriter. 3. In SolrCore.execute(), call a method to copy (if so configured) the explicit request parameters to a nested NamedList stored in the above "headers" NamedList. If people need more than the explicit parameters, this would be the place to change later on, but right now this will be explicit params only. 4. In QueryResponseWriters, implement generic code to copy the "headers" NamedList to the output I think this puts everything in the right place - comments are welcome of course.
          Hide
          Bertrand Delacretaz added a comment -

          Here's a new patch (SOLR-59-20061102.patch and SOLR-59-new-files-20061102.tar.gz).

          I have added a SolrQueryResponseHeaders class to handle these (and any future) additional information fields in the response headers.

          The XML output format is a bit different, example for

          http://localhost:8983/solr/select/?stylesheet=&q=apache&version=2.1&start=0&rows=10&indent=on&multi=red&multi=blue

          <responseHeader>
          <status>0</status>
          <QTime>0</QTime>
          <lst name="addInfo">
          <lst name="explicitQueryParameters">
          <arr name="multi">
          <str>red</str>
          <str>blue</str>
          </arr>
          <str name="rows">10</str>
          <str name="start">0</str>
          <str name="indent">on</str>
          <str name="q">apache</str>
          <str name="stylesheet"/>
          <str name="version">2.1</str>
          </lst>
          </lst>
          </responseHeader>

          And the same info is available in a similar way in the JSON, Python and Ruby response writers.

          The explicitQueryParameters list is activated by the following (optional) parameter in solrconfig.xml, which defaults to false:

          <response>
          <!-- Copy explicit request parameters to response? (SOLR-59) -->
          <echoParameters explicit="true"/>
          </response>

          Default parameters are not dumped for now, adding them should only require changing SolrQueryResponseHeaders.setStandardHeaders().

          Also, DefaultSolrParams.getParameterNamesIterator() now returns all parameters thanks to the new IteratorChain utility class (is there a better way without copying collections? sounds fairly hairy for a simple thing).

          Show
          Bertrand Delacretaz added a comment - Here's a new patch ( SOLR-59 -20061102.patch and SOLR-59 -new-files-20061102.tar.gz). I have added a SolrQueryResponseHeaders class to handle these (and any future) additional information fields in the response headers. The XML output format is a bit different, example for http://localhost:8983/solr/select/?stylesheet=&q=apache&version=2.1&start=0&rows=10&indent=on&multi=red&multi=blue <responseHeader> <status>0</status> <QTime>0</QTime> <lst name="addInfo"> <lst name="explicitQueryParameters"> <arr name="multi"> <str>red</str> <str>blue</str> </arr> <str name="rows">10</str> <str name="start">0</str> <str name="indent">on</str> <str name="q">apache</str> <str name="stylesheet"/> <str name="version">2.1</str> </lst> </lst> </responseHeader> And the same info is available in a similar way in the JSON, Python and Ruby response writers. The explicitQueryParameters list is activated by the following (optional) parameter in solrconfig.xml, which defaults to false: <response> <!-- Copy explicit request parameters to response? ( SOLR-59 ) --> <echoParameters explicit="true"/> </response> Default parameters are not dumped for now, adding them should only require changing SolrQueryResponseHeaders.setStandardHeaders(). Also, DefaultSolrParams.getParameterNamesIterator() now returns all parameters thanks to the new IteratorChain utility class (is there a better way without copying collections? sounds fairly hairy for a simple thing).
          Hide
          Bertrand Delacretaz added a comment -

          New files for SOLR-59-20061102.patch

          Show
          Bertrand Delacretaz added a comment - New files for SOLR-59 -20061102.patch
          Hide
          Yonik Seeley added a comment -

          Looking good Bertrand!

          I think it might make more sense to use the normal query parameter mechanism to control this (requestHandler defaults in solrconfig). That way we can reuse all of the parameter code we already have, we avoid adding another element to solrconfig.xml, and most of all, we can change the behavior per-request (very useful for debugging).

          Also, why the extra addInfo element in the responseHeader? I think I liked it better the way you had it before.

          Side note/question: should we make responseHeader consistent with the generic response format?
          <lst name="responseHeader">
          <int name="status">0</int>
          <int name="QTime">10</int>
          That would be a separate issue of course...

          Show
          Yonik Seeley added a comment - Looking good Bertrand! I think it might make more sense to use the normal query parameter mechanism to control this (requestHandler defaults in solrconfig). That way we can reuse all of the parameter code we already have, we avoid adding another element to solrconfig.xml, and most of all, we can change the behavior per-request (very useful for debugging). Also, why the extra addInfo element in the responseHeader? I think I liked it better the way you had it before. Side note/question: should we make responseHeader consistent with the generic response format? <lst name="responseHeader"> <int name="status">0</int> <int name="QTime">10</int> That would be a separate issue of course...
          Hide
          Bertrand Delacretaz added a comment -

          > ...I think it might make more sense to use the normal query parameter
          > mechanism to control this (requestHandler defaults in solrconfig). ..

          I agree, but I'm not sure how to access these values, are they provided somewhere in SolrCore, or how should I best get them?

          > ...Also, why the extra addInfo element in the responseHeader?..

          Right, makes no sense. This modifed patch goes back to the previous format (still needs the additional files of attachment 3).

          <responseHeader>
          <status>0</status>
          <QTime>1</QTime>
          <lst name="explicitQueryParameters">
          <str name="rows">10</str>
          ...

          > ...should we make responseHeader consistent with the generic response format?

          I think so. I didn't want to change that as it changes the existing format, but it makes sense.

          Show
          Bertrand Delacretaz added a comment - > ...I think it might make more sense to use the normal query parameter > mechanism to control this (requestHandler defaults in solrconfig). .. I agree, but I'm not sure how to access these values, are they provided somewhere in SolrCore, or how should I best get them? > ...Also, why the extra addInfo element in the responseHeader?.. Right, makes no sense. This modifed patch goes back to the previous format (still needs the additional files of attachment 3). <responseHeader> <status>0</status> <QTime>1</QTime> <lst name="explicitQueryParameters"> <str name="rows">10</str> ... > ...should we make responseHeader consistent with the generic response format? I think so. I didn't want to change that as it changes the existing format, but it makes sense.
          Hide
          Yonik Seeley added a comment -

          > I agree, but I'm not sure how to access these values, are they provided somewhere in SolrCore, or how should I best get them?

          You must have them... you added them to the header
          After a request handler is called, it will have added default parameters to the SolrQueryRequest oblect.... so you can just use req.getParams().get("echoParams")

          This could be done in SolrCore.exec() or in a utility function that all ResponseWriters call to get the header data.

          I just peeked at your code now...
          An alternative to adding additional methods to SolrQueryResponse to hold the headers is to just treat the headers like any other data object in the response. That would also allow request handlers to add stuff to the header in the future (if they really needed to):

          So SolrCore.execute could look something like this perhaps:

          public void execute(SolrQueryRequest req, SolrQueryResponse rsp) {
          SolrRequestHandler handler = getRequestHandler(req.getQueryType());
          if (handler==null)

          { log.warning("Unknown Request Handler '" + req.getQueryType() +"' :" + req); throw new SolrException(400,"Unknown Request Handler '" + req.getQueryType() + "'", true); }

          NamedList responseHeader = new NamedList();
          rsp.add("responseHeader", responseHeader);
          handler.handleRequest(req,rsp);
          int qtime=(int)(rsp.getEndTime() - req.getStartTime());
          // might want to re-get responseHeader here, or mandate that request handers
          // do not replace the responseHeader in the SolrQueryResponse they are handed.
          responseHeader.add("status",rsp.getException()==null ? 0 : 500);
          responseHeader.add("QTime",qtime);
          // Values for echoParams... false/true/all or false/explicit/all ???
          if ("explicit".equals(req.getParams().get("echoParams")))
          responseHeader.add("params", asNamedList_TBD(req.getOriginalParams()));
          log.info(req.getParamString()+ " 0 " + qtime);
          }

          Show
          Yonik Seeley added a comment - > I agree, but I'm not sure how to access these values, are they provided somewhere in SolrCore, or how should I best get them? You must have them... you added them to the header After a request handler is called, it will have added default parameters to the SolrQueryRequest oblect.... so you can just use req.getParams().get("echoParams") This could be done in SolrCore.exec() or in a utility function that all ResponseWriters call to get the header data. I just peeked at your code now... An alternative to adding additional methods to SolrQueryResponse to hold the headers is to just treat the headers like any other data object in the response. That would also allow request handlers to add stuff to the header in the future (if they really needed to): So SolrCore.execute could look something like this perhaps: public void execute(SolrQueryRequest req, SolrQueryResponse rsp) { SolrRequestHandler handler = getRequestHandler(req.getQueryType()); if (handler==null) { log.warning("Unknown Request Handler '" + req.getQueryType() +"' :" + req); throw new SolrException(400,"Unknown Request Handler '" + req.getQueryType() + "'", true); } NamedList responseHeader = new NamedList(); rsp.add("responseHeader", responseHeader); handler.handleRequest(req,rsp); int qtime=(int)(rsp.getEndTime() - req.getStartTime()); // might want to re-get responseHeader here, or mandate that request handers // do not replace the responseHeader in the SolrQueryResponse they are handed. responseHeader.add("status",rsp.getException()==null ? 0 : 500); responseHeader.add("QTime",qtime); // Values for echoParams... false/true/all or false/explicit/all ??? if ("explicit".equals(req.getParams().get("echoParams"))) responseHeader.add("params", asNamedList_TBD(req.getOriginalParams())); log.info(req.getParamString()+ " 0 " + qtime); }
          Hide
          Bertrand Delacretaz added a comment -

          Here's a new patch (the newfiles follow, also marked with 20061106).

          You're right that setting the headers like any other data object in the response makes things much simpler and consistent, at the expense of a change of output format, example for the XML:

          <response>
          <lst name="responseHeader">
          <int name="status">0</int>
          <int name="QTime">0</int>
          <lst name="params">
          <str name="rows">10</str>
          <str name="start">0</str>
          . . .
          </lst>
          </lst>

          I have add the EchoParamsTest to check the XML output.

          Show
          Bertrand Delacretaz added a comment - Here's a new patch (the newfiles follow, also marked with 20061106). You're right that setting the headers like any other data object in the response makes things much simpler and consistent, at the expense of a change of output format, example for the XML: <response> <lst name="responseHeader"> <int name="status">0</int> <int name="QTime">0</int> <lst name="params"> <str name="rows">10</str> <str name="start">0</str> . . . </lst> </lst> I have add the EchoParamsTest to check the XML output.
          Hide
          Bertrand Delacretaz added a comment -

          To avoid confusion: the current version of my patch is in these attachments:

          SOLR-59-20061106-newfiles.tar.gz
          SOLR-59-20061106.patch

          I don't know if there's a way to mark attachments as obsolete in Jira.

          Show
          Bertrand Delacretaz added a comment - To avoid confusion: the current version of my patch is in these attachments: SOLR-59 -20061106-newfiles.tar.gz SOLR-59 -20061106.patch I don't know if there's a way to mark attachments as obsolete in Jira.
          Hide
          Yonik Seeley added a comment -

          +1 on the more consistent responseHeader, but perhaps we should bump the current version to 2.2, and stick with the old format if version<2.2 ?

          Show
          Yonik Seeley added a comment - +1 on the more consistent responseHeader, but perhaps we should bump the current version to 2.2, and stick with the old format if version<2.2 ?
          Hide
          Bertrand Delacretaz added a comment -

          +1 for bumping the version number.

          By "stick with the old format if version<2.2" do you mean having code that reproduces the old format if the "version" parameter is set to 2.1?

          IMHO maintaining both formats would would be overkill for a small change like this one...but I don't (yet) have Solr instances in production yet.

          Show
          Bertrand Delacretaz added a comment - +1 for bumping the version number. By "stick with the old format if version<2.2" do you mean having code that reproduces the old format if the "version" parameter is set to 2.1? IMHO maintaining both formats would would be overkill for a small change like this one...but I don't (yet) have Solr instances in production yet.
          Hide
          Yonik Seeley added a comment -

          > but I don't (yet) have Solr instances in production yet.

          We've had Solr in production since June 2005, and responseHeader hasn't changed since then.
          I can help stick in some backward compatible code if we decide it should be there.

          Luckily, because of the simplicity of the JSON format, it's response format is unchanged.

          Show
          Yonik Seeley added a comment - > but I don't (yet) have Solr instances in production yet. We've had Solr in production since June 2005, and responseHeader hasn't changed since then. I can help stick in some backward compatible code if we decide it should be there. Luckily, because of the simplicity of the JSON format, it's response format is unchanged.
          Hide
          Bertrand Delacretaz added a comment -

          Ah ok, as the JSON format hasn't changed, I guess it just needs recreating this in the XMLWriter:

          <responseHeader>
          <status>0</status>
          <QTime>1234</QTime>
          </responseHeader>

          When version < 2.2

          I probably won't have time to do that today or tomorrow, but of course feel free to "patch my patch" if you want.

          Show
          Bertrand Delacretaz added a comment - Ah ok, as the JSON format hasn't changed, I guess it just needs recreating this in the XMLWriter: <responseHeader> <status>0</status> <QTime>1234</QTime> </responseHeader> When version < 2.2 I probably won't have time to do that today or tomorrow, but of course feel free to "patch my patch" if you want.
          Hide
          Bill Au added a comment -

          >I can help stick in some backward compatible code if we decide it should be there.

          +1 on keeping things backward compatible.

          Show
          Bill Au added a comment - >I can help stick in some backward compatible code if we decide it should be there. +1 on keeping things backward compatible.
          Hide
          Yonik Seeley added a comment -

          Ok, updated patch to

          • bump current & default version to 2.2
          • bump version used in tests to 2.2 (added 2.0 to tests where necessary)
          • changed "name" of the normal doclist returned by the handlers to "response"
          • other little minor cleanups

          Bertrand, since you started this issue, I'll let you commit it if you feel comfortable with it

          Show
          Yonik Seeley added a comment - Ok, updated patch to bump current & default version to 2.2 bump version used in tests to 2.2 (added 2.0 to tests where necessary) changed "name" of the normal doclist returned by the handlers to "response" other little minor cleanups Bertrand, since you started this issue, I'll let you commit it if you feel comfortable with it
          Hide
          Bertrand Delacretaz added a comment -

          Thanks Yonik, I have commited your patch.

          I have renamed the OutputWriterTest.testOriginalSolrWriter test to testSOLR59responseHeaderVersions, and added a test where the "version"parameter is not used.

          I think you can close this issue (apparently I don't have sufficient permissions to do that).

          Show
          Bertrand Delacretaz added a comment - Thanks Yonik, I have commited your patch. I have renamed the OutputWriterTest.testOriginalSolrWriter test to testSOLR59responseHeaderVersions, and added a test where the "version"parameter is not used. I think you can close this issue (apparently I don't have sufficient permissions to do that).
          Hide
          Bertrand Delacretaz added a comment -

          Let's try my brand new Jira karma by resolving this...

          Show
          Bertrand Delacretaz added a comment - Let's try my brand new Jira karma by resolving this...
          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:
              Unassigned
              Reporter:
              Bertrand Delacretaz
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development