Improve error handling in controller #14951
Conversation
Introduce a new PinotRuntimeException and modify other SPI exceptions to extend from it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14951 +/- ##
============================================
+ Coverage 61.75% 63.71% +1.96%
- Complexity 207 1469 +1262
============================================
Files 2436 2712 +276
Lines 133233 151943 +18710
Branches 20636 23463 +2827
============================================
+ Hits 82274 96807 +14533
- Misses 44911 47865 +2954
- Partials 6048 7271 +1223
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java
Show resolved
Hide resolved
| public BadQueryRequestException(String message) { | ||
| super(message); | ||
| super(SQL_RUNTIME_ERROR_CODE, message); |
There was a problem hiding this comment.
This class seems to be used for user errors - why do we want to use SQL_RUNTIME_ERROR_CODE for that?
There was a problem hiding this comment.
This class is used in tons of places to detect errors in runtime. There are places where it is being caught and reconverted into a different error type depending on the context where it was fired. For example BaseSingleBlockCombineOperator.
Here, I'm assigning a default error code, which, in general, is something difficult to do in a precise manner. Callers can anyway include an explicit error code
...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/exception/QueryException.java
Show resolved
Hide resolved
| } catch (QException e) { | ||
| if (e.getErrorCode() != QueryException.UNKNOWN_ERROR_CODE) { | ||
| throw e; | ||
| } else { | ||
| throw new QException(QException.SQL_PARSING_ERROR_CODE, e); | ||
| } | ||
| } catch (Exception e) { | ||
| return QueryException.getException(QueryException.SQL_PARSING_ERROR, | ||
| new Exception("Unable to find table for this query", e)).toString(); | ||
| throw new QException(QException.SQL_PARSING_ERROR_CODE, e); |
There was a problem hiding this comment.
This part is a little confusing - why are we treating unknown error as parsing error but not handling other types of QException here?
There was a problem hiding this comment.
Because we assume other error types will be more precise. The error codes being used are pretty chaotic. It looks like we added them without actual consideration and the fact that we cannot create hierarchies is not a good design. For example, is a UNKNOWN_COLUMN_ERROR_CODE a QUERY_VALIDATION_ERROR_CODE?
...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
Show resolved
Hide resolved
...t-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java
Show resolved
Hide resolved
|
@yashmayya I've created #14994, which is built on top of this PR and fixes most of the changes reported here |
|
This has been superseded by #15277 |
This PR is trying to fix the two first controller errors listed in #14950:
It also optimizes the way controllers pipeline query results from brokers. The current version should be faster (see https://schoeffm.github.io/posts/response-streaming-between-jaxrs-and-webcomponents-part1/) and, most importantly, zero-copy (while previous code copied the query response on the heap not once but twice!!!)