Solr
  1. Solr
  2. SOLR-4487

SolrException usage in solrj client code can't handle all possible http error codes returned by servers -- example "413 Request Entity Too Large"

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0
    • Fix Version/s: 4.3, 6.0
    • Component/s: clients - java
    • Labels:
      None

      Description

      Solr responds to excessively large queries with a 413 status code, but HttpSolrServer.request() loses this information when it tries to look up the code in SolrException.ErrorCode, resulting in a status code 0 in the thrown exception.

      Being able to see this status code would be helpful.

        Activity

        Hide
        Hoss Man added a comment -

        Once upon a time (before SolrJ even existed), SolrException's constructor took in an arbitrary numeric code, and it was being used inconsistently, so the ErrorCode enum was defined to help ensure that when errors were thrown by solr, they were thrown using a consistent, finite subset, of http status codes that could be propagate to the end user.

        But once solrj was added, and we started using SolrException in clients to wrap any and all errors returned by Solr via HTTP, i think it's a mistake to continue limiting SolrException to this enumeration.

        Solr as a server should still have a finite, limited list of error codes that it throws, but if the SolrException is going to be used in the client, then it needs to be able to handle all of the possible status codes that might be returned by any arbitrary http server (or proxy) that the client is talking to.

        The really ironic thing is that SolrException still tracks & exposes the status code as an arbitrary int (via the code() method) – it's only the constructor that limits you to the ErrorCode enum.

        So i propose we re-add a constructor to SolrException that accepts an arbitrary int error code, and start using that in client code like HttpSolrServer where we are building up an exception from an arbitrary http server response.

        The addition is trivial, but we should obviously add some javadocs explaining when/why to use each constructor...

          public SolrException(int code, String msg, Throwable th) {
            super(msg, th);
            this.code = code;
          }
        

        ...any objections?

        alternative suggestion: add a package protected (or private?) subclass of SolrException to org.apache.solr.client.impl (or maybe even directly in HttpSolrServer) and put this constructor there.

        i actually think i kind of like this alternative idea better, because i would help mitigate the risk of people using the int constructor with bogus error code values in other solr code.

        Show
        Hoss Man added a comment - Once upon a time (before SolrJ even existed), SolrException's constructor took in an arbitrary numeric code, and it was being used inconsistently, so the ErrorCode enum was defined to help ensure that when errors were thrown by solr, they were thrown using a consistent, finite subset, of http status codes that could be propagate to the end user. But once solrj was added, and we started using SolrException in clients to wrap any and all errors returned by Solr via HTTP, i think it's a mistake to continue limiting SolrException to this enumeration. Solr as a server should still have a finite, limited list of error codes that it throws, but if the SolrException is going to be used in the client, then it needs to be able to handle all of the possible status codes that might be returned by any arbitrary http server (or proxy) that the client is talking to. The really ironic thing is that SolrException still tracks & exposes the status code as an arbitrary int (via the code() method) – it's only the constructor that limits you to the ErrorCode enum. So i propose we re-add a constructor to SolrException that accepts an arbitrary int error code, and start using that in client code like HttpSolrServer where we are building up an exception from an arbitrary http server response. The addition is trivial, but we should obviously add some javadocs explaining when/why to use each constructor... public SolrException(int code, String msg, Throwable th) { super(msg, th); this.code = code; } ...any objections? alternative suggestion: add a package protected (or private?) subclass of SolrException to org.apache.solr.client.impl (or maybe even directly in HttpSolrServer) and put this constructor there. i actually think i kind of like this alternative idea better, because i would help mitigate the risk of people using the int constructor with bogus error code values in other solr code.
        Hide
        Alexander Dietrich added a comment -

        Sounds good to me. In addition, SolrException.code() could use some documentation, so people know that they're getting an HTTP status code (or 0).

        Show
        Alexander Dietrich added a comment - Sounds good to me. In addition, SolrException.code() could use some documentation, so people know that they're getting an HTTP status code (or 0).
        Hide
        Hoss Man added a comment -

        Patch with fix and tests.

        holding off on committing just yet because some other HttpSolrServer changes that just got committed seem to have broken bunch of tests, waiting for things to return to stead state.

        Show
        Hoss Man added a comment - Patch with fix and tests. holding off on committing just yet because some other HttpSolrServer changes that just got committed seem to have broken bunch of tests, waiting for things to return to stead state.
        Hide
        Hoss Man added a comment -

        Committed revision 1468705.
        Committed revision 1468718.

        Show
        Hoss Man added a comment - Committed revision 1468705. Committed revision 1468718.
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            Hoss Man
            Reporter:
            Alexander Dietrich
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development