Solr
  1. Solr
  2. SOLR-7243

4.10.3 SolrJ is throwing a SERVER_ERROR exception instead of BAD_REQUEST

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 4.10.3
    • Fix Version/s: 5.2, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      We found this problem while upgrading Solr from 4.4 to 4.10.3. Our integration test is similar to this Solr unit test,

      https://github.com/apache/lucene-solr/blob/trunk/solr/core/src/test/org/apache/solr/schema/TestCloudSchemaless.java

      Specifically we test if the Solr server returns BAD_REQUEST when provided with incorrect input.The only difference is that it uses CloudSolrServer instead of HttpSolrServer. The CloudSolrServer always returns SERVER_ERROR error code. Please take a look

      https://github.com/apache/lucene-solr/blob/817303840fce547a1557e330e93e5a8ac0618f34/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrServer.java#L359

      I think we can improve the error handling by checking if the first exception in the list is of type SolrException and if that is the case return the error code associated with that exception. If the first exception is not of type SolrException, then we can return SERVER_ERROR code.

      1. SOLR-7243.patch
        4 kB
        Shawn Heisey
      2. SOLR-7243.patch
        3 kB
        Hrishikesh Gadre
      3. SOLR-7243.patch
        3 kB
        Hrishikesh Gadre
      4. SOLR-7243.patch
        1 kB
        Hrishikesh Gadre
      5. SOLR-7243.patch
        1 kB
        Hrishikesh Gadre

        Activity

        Hide
        Shawn Heisey added a comment -

        This looks reasonable to me.

        Show
        Shawn Heisey added a comment - This looks reasonable to me.
        Hide
        Hrishikesh Gadre added a comment -

        Shawn Heisey A new patch against the trunk.

        Improved the error handling such that
        1. error code corresponding to first exception is used (if possible) while throwing the RouteException.
        2. Ensure that the SolrServerException does not gobble up the SolrException (which contains the actual error code). Note that as per the documentation, SolrServerException should be used only for communication / parsing issues associated with talking to SOLR. Hence it can not represent valid errors returned by the Solr server (since it does not have error-code associated with it).

        Also added a unit test to verify the changes.

        Show
        Hrishikesh Gadre added a comment - Shawn Heisey A new patch against the trunk. Improved the error handling such that 1. error code corresponding to first exception is used (if possible) while throwing the RouteException. 2. Ensure that the SolrServerException does not gobble up the SolrException (which contains the actual error code). Note that as per the documentation, SolrServerException should be used only for communication / parsing issues associated with talking to SOLR. Hence it can not represent valid errors returned by the Solr server (since it does not have error-code associated with it). Also added a unit test to verify the changes.
        Hide
        Hrishikesh Gadre added a comment -

        Accidentally uploaded old patch file. Please ignore it (although the comment is still valid).

        Show
        Hrishikesh Gadre added a comment - Accidentally uploaded old patch file. Please ignore it (although the comment is still valid).
        Hide
        Shawn Heisey added a comment -

        Initial comments, after a quick glance and before I have applied the patch and tried to digest it:

        In a couple of places, you are casting an exception to SolrException .. but in both cases, because of the way the code is written, you can already be assured that the exception is a SolrException (or a derivative class). Is a cast really necessary?

        Show
        Shawn Heisey added a comment - Initial comments, after a quick glance and before I have applied the patch and tried to digest it: In a couple of places, you are casting an exception to SolrException .. but in both cases, because of the way the code is written, you can already be assured that the exception is a SolrException (or a derivative class). Is a cast really necessary?
        Hide
        Hrishikesh Gadre added a comment -

        Shawn Heisey yeah good point. Let me fix this ASAP.

        Show
        Hrishikesh Gadre added a comment - Shawn Heisey yeah good point. Let me fix this ASAP.
        Hide
        Hrishikesh Gadre added a comment -

        Shawn Heisey I removed the cast from one place (couldn't remove the other one since compiler complains. Note the method signature does not allow throwing generic Exceptions in that case).

        Show
        Hrishikesh Gadre added a comment - Shawn Heisey I removed the cast from one place (couldn't remove the other one since compiler complains. Note the method signature does not allow throwing generic Exceptions in that case).
        Hide
        Hrishikesh Gadre added a comment -

        Shawn Heisey Did you get a chance to review the patch? Please let me know if any feedback.

        Show
        Hrishikesh Gadre added a comment - Shawn Heisey Did you get a chance to review the patch? Please let me know if any feedback.
        Hide
        Shawn Heisey added a comment -

        I have not yet had a chance to look deeper. $JOB and $REAL_LIFE get in the way.

        Show
        Shawn Heisey added a comment - I have not yet had a chance to look deeper. $JOB and $REAL_LIFE get in the way.
        Hide
        Shawn Heisey added a comment -

        Updated patch. Still need to run tests and precommit.

        Show
        Shawn Heisey added a comment - Updated patch. Still need to run tests and precommit.
        Hide
        Shawn Heisey added a comment -

        Tests and precommit on trunk passed.

        I think this is a good change, but I'm curious what others think.

        Show
        Shawn Heisey added a comment - Tests and precommit on trunk passed. I think this is a good change, but I'm curious what others think.
        Hide
        Hrishikesh Gadre added a comment -

        Shawn Heisey I don't see any negative response for this. Should we commit?

        Show
        Hrishikesh Gadre added a comment - Shawn Heisey I don't see any negative response for this. Should we commit?
        Hide
        ASF subversion and git services added a comment -

        Commit 1679099 from Shawn Heisey in branch 'dev/trunk'
        [ https://svn.apache.org/r1679099 ]

        SOLR-7243: Return more informative error from CloudSolrServer when available.

        Show
        ASF subversion and git services added a comment - Commit 1679099 from Shawn Heisey in branch 'dev/trunk' [ https://svn.apache.org/r1679099 ] SOLR-7243 : Return more informative error from CloudSolrServer when available.
        Hide
        ASF subversion and git services added a comment -

        Commit 1679122 from Shawn Heisey in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1679122 ]

        SOLR-7243: Return more informative error from CloudSolrServer when available. (merge trunk r1679099)

        Show
        ASF subversion and git services added a comment - Commit 1679122 from Shawn Heisey in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1679122 ] SOLR-7243 : Return more informative error from CloudSolrServer when available. (merge trunk r1679099)
        Hide
        Shawn Heisey added a comment -

        Tests and precommit are good.

        Show
        Shawn Heisey added a comment - Tests and precommit are good.
        Hide
        Anshum Gupta added a comment -

        Bulk close for 5.2.0.

        Show
        Anshum Gupta added a comment - Bulk close for 5.2.0.

          People

          • Assignee:
            Shawn Heisey
            Reporter:
            Hrishikesh Gadre
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development