Uploaded image for project: 'Calcite'
  1. Calcite
  2. CALCITE-645

Pass server-side exceptions back to the client

    Details

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

      Description

      Avatica RPC response objects should contain an exception field that can be deserialized and re-thrown on the client side. That way client sees stack traces that are more meaningful than "500 error".

        Issue Links

          Activity

          Hide
          julianhyde Julian Hyde added a comment -

          Agreed. Ultimately the JDBC client driver should throw a SQLException or sub-class. The JDBC client has a bad habit of throwing RuntimeExceptions right now.

          When re-constructing the chain of exceptions, I don't recommend that you try to create exceptions of the same type. The type may not exist on the client. As long as the call stack string comes through, we should be OK.

          Show
          julianhyde Julian Hyde added a comment - Agreed. Ultimately the JDBC client driver should throw a SQLException or sub-class. The JDBC client has a bad habit of throwing RuntimeExceptions right now. When re-constructing the chain of exceptions, I don't recommend that you try to create exceptions of the same type. The type may not exist on the client. As long as the call stack string comes through, we should be OK.
          Hide
          ndimiduk Nick Dimiduk added a comment -

          Agreed.

          Any thoughts on how this might work for non-Java clients. ODBC, Python, and C# come to immediate mind.

          Show
          ndimiduk Nick Dimiduk added a comment - Agreed. Any thoughts on how this might work for non-Java clients. ODBC, Python, and C# come to immediate mind.
          Hide
          julianhyde Julian Hyde added a comment -

          I presume each client API (e.g. ODBC) has a way to propagate errors. The information content probably differs but has at its core an error message, error code, severity (e.g. error versus warning) and maybe a list of causing errors and stack dump as string. I think a JSON-ified SQLException (including its "next" and "cause" exceptions, SQLcode, sub-class, stack trace) will serve these APIs too.

          Might be worth a quick look at the ODBC spec to validate. Everything else is likely built on ODBC or JDBC.

          Show
          julianhyde Julian Hyde added a comment - I presume each client API (e.g. ODBC) has a way to propagate errors. The information content probably differs but has at its core an error message, error code, severity (e.g. error versus warning) and maybe a list of causing errors and stack dump as string. I think a JSON-ified SQLException (including its "next" and "cause" exceptions, SQLcode, sub-class, stack trace) will serve these APIs too. Might be worth a quick look at the ODBC spec to validate. Everything else is likely built on ODBC or JDBC.
          Hide
          ndimiduk Nick Dimiduk added a comment -

          Bulk edit assigning avatica component to obvious issues.

          Show
          ndimiduk Nick Dimiduk added a comment - Bulk edit assigning avatica component to obvious issues.
          Hide
          elserj Josh Elser added a comment -

          I started poking at this one myself over the weekend.

          Might be worth a quick look at the ODBC spec to validate

          Do you have specifics you could point me at, Julian Hyde? I would appreciate it.

          Show
          elserj Josh Elser added a comment - I started poking at this one myself over the weekend. Might be worth a quick look at the ODBC spec to validate Do you have specifics you could point me at, Julian Hyde ? I would appreciate it.
          Hide
          julianhyde Julian Hyde added a comment -

          I found ODBC Diagnostics & Error Status Codes, which gives you an idea how to get multiple error or warning records back from an ODBC statement, with a sqlstate, error code, and message. The data model is basically the same as JDBC returning a list of SQLException objects, so the data passing over the Avatica protocol should look similar.

          Show
          julianhyde Julian Hyde added a comment - I found ODBC Diagnostics & Error Status Codes , which gives you an idea how to get multiple error or warning records back from an ODBC statement, with a sqlstate, error code, and message. The data model is basically the same as JDBC returning a list of SQLException objects, so the data passing over the Avatica protocol should look similar.
          Hide
          julianhyde Julian Hyde added a comment -

          Note that Bruno Dumon's patch to CALCITE-912 does some work on exception handling.

          Show
          julianhyde Julian Hyde added a comment - Note that Bruno Dumon 's patch to CALCITE-912 does some work on exception handling.
          Hide
          elserj Josh Elser added a comment -

          The data model is basically the same as JDBC returning a list of SQLException objects, so the data passing over the Avatica protocol should look similar.

          Great. Thanks for that pointer. I've been a little mystified about what ODBC actually looks like (aside from what I can see in Hive)

          Note that Bruno Dumon's patch to CALCITE-912 does some work on exception handling.

          Thanks, I hadn't paid close enough attention to the changes there. I'll take a look.

          Show
          elserj Josh Elser added a comment - The data model is basically the same as JDBC returning a list of SQLException objects, so the data passing over the Avatica protocol should look similar. Great. Thanks for that pointer. I've been a little mystified about what ODBC actually looks like (aside from what I can see in Hive) Note that Bruno Dumon's patch to CALCITE-912 does some work on exception handling. Thanks, I hadn't paid close enough attention to the changes there. I'll take a look.
          Hide
          elserj Josh Elser added a comment -

          First attempt at changes to support passing back server error information to the client and transforming it back to a SQLException.

          Show
          elserj Josh Elser added a comment - First attempt at changes to support passing back server error information to the client and transforming it back to a SQLException.
          Hide
          elserj Josh Elser added a comment -

          Looking at these changes again, I noticed that I ignored the comment about the notion of "chained" SQLExceptions. I'm trying to understand what this would look like in Avatica (much less an example of some "normal" JDBC driver). Suggestions, anyone?

          Show
          elserj Josh Elser added a comment - Looking at these changes again, I noticed that I ignored the comment about the notion of "chained" SQLExceptions. I'm trying to understand what this would look like in Avatica (much less an example of some "normal" JDBC driver). Suggestions, anyone?
          Hide
          julianhyde Julian Hyde added a comment -

          IIRC, JDBC has the concept of a causal chain (which all Java exceptions now have) and it also has the concept of a list of exceptions not related to each other. The typical use case for the latter is warnings about rows rejected during a load, or rows that had arithmetic exceptions during query processing.

          I think of SQLException.getCause() as an "upwards" link and SQLException.getNextException() as as "sideways" link. The chain of exceptions by getNextException() must all be instances of SQLException.

          I would ignore for now the possibility that the cause of a SQLException is itself a SQLException that is part of a chain. Thus the structure looks like this:

            e1c2                e3c2
             |                   |
            e1c1                e3c1      e4c1
             |                   |        |
            e1 ------ e2 ------ e3 ------ e4
          

          I think I'd model the chain of exceptions from a statement as a list of exceptions, and each exception has a "cause" field which is either null or a serialized copy of the getCause() exception.

          Show
          julianhyde Julian Hyde added a comment - IIRC, JDBC has the concept of a causal chain (which all Java exceptions now have) and it also has the concept of a list of exceptions not related to each other. The typical use case for the latter is warnings about rows rejected during a load, or rows that had arithmetic exceptions during query processing. I think of SQLException.getCause() as an "upwards" link and SQLException.getNextException() as as "sideways" link. The chain of exceptions by getNextException() must all be instances of SQLException. I would ignore for now the possibility that the cause of a SQLException is itself a SQLException that is part of a chain. Thus the structure looks like this: e1c2 e3c2 | | e1c1 e3c1 e4c1 | | | e1 ------ e2 ------ e3 ------ e4 I think I'd model the chain of exceptions from a statement as a list of exceptions, and each exception has a "cause" field which is either null or a serialized copy of the getCause() exception.
          Hide
          elserj Josh Elser added a comment -

          Thanks for the explanation, Julian. This was very helpful.

          I've updated the linked pull request to try to take this into account while also cleaning up some other things I've come to understand better (e.g. types for the error code and SQLState)

          Show
          elserj Josh Elser added a comment - Thanks for the explanation, Julian. This was very helpful. I've updated the linked pull request to try to take this into account while also cleaning up some other things I've come to understand better (e.g. types for the error code and SQLState)
          Hide
          julianhyde Julian Hyde added a comment -

          Please rename AvaticaSQLException and AvaticaSQLExceptionTest to camel case. +1 when that's done

          Show
          julianhyde Julian Hyde added a comment - Please rename AvaticaSQLException and AvaticaSQLExceptionTest to camel case. +1 when that's done
          Hide
          elserj Josh Elser added a comment -

          Please rename AvaticaSQLException and AvaticaSQLExceptionTest to camel case

          Done! Thanks for reviewing.

          Show
          elserj Josh Elser added a comment - Please rename AvaticaSQLException and AvaticaSQLExceptionTest to camel case Done! Thanks for reviewing.
          Hide
          julianhyde Julian Hyde added a comment -

          OK, now you need to rebase this and all of the other PRs that modify protobuf - ideally into a single PR with multiple commits. And I guess CALCITE-912 has to be first.

          Show
          julianhyde Julian Hyde added a comment - OK, now you need to rebase this and all of the other PRs that modify protobuf - ideally into a single PR with multiple commits. And I guess CALCITE-912 has to be first.
          Hide
          elserj Josh Elser added a comment -

          Yeah, I can do that. How about I grab this, CALCITE-912 and CALCITE-908, and update https://github.com/apache/incubator-calcite/pull/144 with them. I'll modify my other open PRs working off of the newer protobuf dependency.

          Show
          elserj Josh Elser added a comment - Yeah, I can do that. How about I grab this, CALCITE-912 and CALCITE-908 , and update https://github.com/apache/incubator-calcite/pull/144 with them. I'll modify my other open PRs working off of the newer protobuf dependency.
          Hide
          elserj Josh Elser added a comment -

          Oh, nevermind. I see what you mean now about CALCITE-912. Let me hit up Bruno first.

          Show
          elserj Josh Elser added a comment - Oh, nevermind. I see what you mean now about CALCITE-912 . Let me hit up Bruno first.
          Hide
          julianhyde Julian Hyde added a comment -

          When you have something ready please post to the dev list. I don't always notice JIRA updates in a timely fashion.

          Show
          julianhyde Julian Hyde added a comment - When you have something ready please post to the dev list. I don't always notice JIRA updates in a timely fashion.
          Hide
          julianhyde Julian Hyde added a comment -

          Renaming AvaticaSQLException broke javadoc. Please make sure that 'mvn site' passes under both JDK 1.7 and 1.8.

          Show
          julianhyde Julian Hyde added a comment - Renaming AvaticaSQLException broke javadoc. Please make sure that 'mvn site' passes under both JDK 1.7 and 1.8.
          Hide
          elserj Josh Elser added a comment -

          Renaming AvaticaSQLException broke javadoc. Please make sure that 'mvn site' passes under both JDK 1.7 and 1.8.

          ACKed.

          Show
          elserj Josh Elser added a comment - Renaming AvaticaSQLException broke javadoc. Please make sure that 'mvn site' passes under both JDK 1.7 and 1.8. ACKed.
          Show
          julianhyde Julian Hyde added a comment - Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/5be93fb4 . Thanks for the PR, Josh Elser !
          Hide
          jcamachorodriguez Jesus Camacho Rodriguez added a comment -

          Resolved in release 1.5.0 (2015-11-10)

          Show
          jcamachorodriguez Jesus Camacho Rodriguez added a comment - Resolved in release 1.5.0 (2015-11-10)

            People

            • Assignee:
              elserj Josh Elser
              Reporter:
              ndimiduk Nick Dimiduk
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development