Solr
  1. Solr
  2. SOLR-505

Give RequestHandlers the possiblity to suppress the generation of HTTP caching headers

    Details

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

      Description

      The code from SOLR-127 emits HTTP cache headers for all handlers if configured. We should not emit cache related headers for update request handlers. Partial responses (coming from the Timeout request stuff) should not be cached as well.

      To solve this problem we can simply add two methods to the SolrQueryResponse class (like void setAvoidHTTPCaching(boolean) and boolean isAvoidHTTPCaching() - the default for the value would be false). The update request handlers should set this to true all the time. The partial response stuff can set this to true as well.

      1. SOLR-505.patch
        14 kB
        Thomas Peuss
      2. SOLR-505.patch
        10 kB
        Thomas Peuss
      3. SOLR-505.patch
        8 kB
        Thomas Peuss

        Issue Links

          Activity

          Hide
          Thomas Peuss added a comment -

          Draft version of a patch.

          Show
          Thomas Peuss added a comment - Draft version of a patch.
          Hide
          Yonik Seeley added a comment -

          +1 on the general approach.
          Distributed search will probably need to avoid caching as well.

          Show
          Yonik Seeley added a comment - +1 on the general approach. Distributed search will probably need to avoid caching as well.
          Hide
          Thomas Peuss added a comment -

          BTW: The patch uses avoidHttpCaching=true as default as this is the safer approach. The patch only set this to false for the search handlers.

          Show
          Thomas Peuss added a comment - BTW: The patch uses avoidHttpCaching=true as default as this is the safer approach. The patch only set this to false for the search handlers.
          Hide
          Sean Timm added a comment -

          Should errors be cached? Certainly some are transient and shouldn't be.

          In the SolrDispatchFilter,

          if( solrRsp.getException() != null ) {
              sendError( (HttpServletResponse)response, solrRsp.getException() );
          }
          

          the Expires and Cache-Control headers could be reset as in HttpCacheHeaderUtil.checkAvoidHttpCaching(...).

          Show
          Sean Timm added a comment - Should errors be cached? Certainly some are transient and shouldn't be. In the SolrDispatchFilter, if ( solrRsp.getException() != null ) { sendError( (HttpServletResponse)response, solrRsp.getException() ); } the Expires and Cache-Control headers could be reset as in HttpCacheHeaderUtil.checkAvoidHttpCaching(...).
          Hide
          Thomas Peuss added a comment - - edited
          • Updated to trunk
          • Emits no-cache headers in case of exceptions
          Show
          Thomas Peuss added a comment - - edited Updated to trunk Emits no-cache headers in case of exceptions
          Hide
          Thomas Peuss added a comment -

          Any further comments on this? Good enough?

          Show
          Thomas Peuss added a comment - Any further comments on this? Good enough?
          Hide
          Sean Timm added a comment -

          It worked well for me. I'd like to see it included in 1.3.

          Show
          Sean Timm added a comment - It worked well for me. I'd like to see it included in 1.3.
          Hide
          Otis Gospodnetic added a comment -

          Yes, I think we should get this in 1.3.
          I took a quick look at the patch and saw this:

          rsp.setAvoidHttpCaching(false);

          Am I the only one who has a harder time reading negative methods like this, esp. when they take false?
          Would it not be nicer to just have:

          rsp.setHttpCaching(true/false);

          or even

          rsp.httpCachingOn() + rsp.httpCachingOff()

          Similarly, instead of

          isAvoidHttpCaching()

          have

          isHttpCachingOn()

          I know this is "just naming", but I think it helps with readability a bit.

          I notice the unit test mods are not in the patch. Is there no need to test the modified behaviour?

          Show
          Otis Gospodnetic added a comment - Yes, I think we should get this in 1.3. I took a quick look at the patch and saw this: rsp.setAvoidHttpCaching( false ); Am I the only one who has a harder time reading negative methods like this, esp. when they take false? Would it not be nicer to just have: rsp.setHttpCaching( true / false ); or even rsp.httpCachingOn() + rsp.httpCachingOff() Similarly, instead of isAvoidHttpCaching() have isHttpCachingOn() I know this is "just naming", but I think it helps with readability a bit. I notice the unit test mods are not in the patch. Is there no need to test the modified behaviour?
          Hide
          Otis Gospodnetic added a comment -

          Thomas - just to make it more obvious - it looks like Sean Timm included your patch in SOLR-502, so I will look at patches provided with SOLR-502 instead of expecting code/patch changes here under SOLR-505. I hope that's OK and that it simplifies things.

          Show
          Otis Gospodnetic added a comment - Thomas - just to make it more obvious - it looks like Sean Timm included your patch in SOLR-502 , so I will look at patches provided with SOLR-502 instead of expecting code/patch changes here under SOLR-505 . I hope that's OK and that it simplifies things.
          Hide
          Thomas Peuss added a comment -

          I don't think that is a good idea to include the patch for SOLR-505 with the patch for SOLR-502. Sean included it only because he depends on the code changes we do with SOLR-505.

          I would like to see the following:

          • Rework SOLR-505 like Otis mentioned. As we only change the "naming" and not the overall approach I am fine with that. I can do that tomorrow.
          • Commit SOLR-505
          • Rework SOLR-502 to use the changed API and to be compatible with trunk
          • Commit SOLR-505

          SOLR-505 and SOLR-502 have different goals. So combining them makes no big sense to me and might confuse people.

          Show
          Thomas Peuss added a comment - I don't think that is a good idea to include the patch for SOLR-505 with the patch for SOLR-502 . Sean included it only because he depends on the code changes we do with SOLR-505 . I would like to see the following: Rework SOLR-505 like Otis mentioned. As we only change the "naming" and not the overall approach I am fine with that. I can do that tomorrow. Commit SOLR-505 Rework SOLR-502 to use the changed API and to be compatible with trunk Commit SOLR-505 SOLR-505 and SOLR-502 have different goals. So combining them makes no big sense to me and might confuse people.
          Hide
          David Smiley added a comment -

          Otis, I completely agree with your comment on the naming. It's a bit odd. I was thinking the same thing before your comment and I should of said something too; don't know why I didn't.

          Show
          David Smiley added a comment - Otis, I completely agree with your comment on the naming. It's a bit odd. I was thinking the same thing before your comment and I should of said something too; don't know why I didn't.
          Hide
          Otis Gospodnetic added a comment -

          Thomas - very much fine with me. I, too, was wondering why Sean incorporated your patch in his, but assumed you guys had a deal of some kind (e.g. perhaps you were colleagues working in sync).
          I'll wait for the new patch, commit, resolve this issue and then I'll move over to SOLR-502 and wait for Sean to make a new and clean patch for that timeout functionality.

          Show
          Otis Gospodnetic added a comment - Thomas - very much fine with me. I, too, was wondering why Sean incorporated your patch in his, but assumed you guys had a deal of some kind (e.g. perhaps you were colleagues working in sync). I'll wait for the new patch, commit, resolve this issue and then I'll move over to SOLR-502 and wait for Sean to make a new and clean patch for that timeout functionality.
          Hide
          Thomas Peuss added a comment -
          • Reworked naming like Otis suggested
          • Some other minor changes
          • Added tests to check the new code (this was the hardest part)
          Show
          Thomas Peuss added a comment - Reworked naming like Otis suggested Some other minor changes Added tests to check the new code (this was the hardest part)
          Hide
          Otis Gospodnetic added a comment -

          Danke, Thomas.

          Committed revision 659657.

          Show
          Otis Gospodnetic added a comment - Danke, Thomas. Committed revision 659657.

            People

            • Assignee:
              Otis Gospodnetic
              Reporter:
              Thomas Peuss
            • Votes:
              3 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development