kfaraz commented on code in PR #13564:
URL: https://github.com/apache/druid/pull/13564#discussion_r1049225215
##########
core/src/main/java/org/apache/druid/query/QueryException.java:
##########
@@ -30,12 +29,126 @@
import java.util.function.Function;
/**
- * Base serializable error response
- *
+ * Base serializable error response.
+ * <p>
+ * The Object Model that QueryException follows is a little non-intuitive as
the primary way that a QueryException is
+ * generated is through a child class. However, those child classes are *not*
equivalent to a QueryException, instead
+ * they act as a Factory of QueryException objects. This can be seen in two
different places.
+ * <p>
+ * 1. When sanitize() is called, the response is a QueryException without any
indication of which original exception
+ * occurred.
+ * 2. When these objects get serialized across the wire the recipient
deserializes a QueryException. The client is
+ * never expected, and fundamentally is not allowed to, ever deserialize a
child class of QueryException.
+ * <p>
+ * For this reason, QueryException must contain all potential state that any
of its child classes could ever want to
+ * push across the wire. Additionally, any catch clauses expecting one of the
child Exceptions must know that it is
+ * running inside of code where the exception has not traveled across the
wire. If there is a chance that the
+ * exception could have been serialized across the wire, the code must catch a
QueryException and check the errorCode
+ * instead.
+ * <p>
+ * As a correlary, adding new state or adjusting the logic of this class must
always be done in a backwards-compatible
+ * fashion across all child classes of QueryException.
+ * <p>
+ * If there is any need to do different logic based on the type of error that
has happened, the only reliable method
+ * of discerning the type of the error is to look at the errorCode String.
Because these Strings are considered part
+ * of the API, they are not allowed to change and must maintain their same
semantics. The known errorCode Strings
+ * are pulled together as public static fields on this class in order to make
it more clear what options exist.
+ * <p>
* QueryResource and SqlResource are expected to emit the JSON form of this
object when errors happen.
*/
public class QueryException extends RuntimeException implements
SanitizableException
{
+ /**
+ * Error codes
+ */
+ public static final String JSON_PARSE_ERROR_CODE = "Json parse failed";
+ public static final String BAD_QUERY_CONTEXT_ERROR_CODE = "Query context
parse failed";
+ public static final String QUERY_CAPACITY_EXCEEDED_ERROR_CODE = "Query
capacity exceeded";
+ public static final String QUERY_INTERRUPTED_ERROR_CODE = "Query
interrupted";
+ // Note: the proper spelling is with a single "l", but the version with
+ // two "l"s is documented, we can't change the text of the message.
+ public static final String QUERY_CANCELED_ERROR_CODE = "Query cancelled";
+ public static final String UNAUTHORIZED_ERROR_CODE = "Unauthorized request";
+ public static final String UNSUPPORTED_OPERATION_ERROR_CODE = "Unsupported
operation";
+ public static final String TRUNCATED_RESPONSE_CONTEXT_ERROR_CODE =
"Truncated response context";
+ public static final String UNKNOWN_EXCEPTION_ERROR_CODE = "Unknown
exception";
+ public static final String QUERY_TIMEOUT_ERROR_CODE = "Query timeout";
+ public static final String QUERY_UNSUPPORTED_ERROR_CODE = "Unsupported
query";
+ public static final String RESOURCE_LIMIT_EXCEEDED_ERROR_CODE = "Resource
limit exceeded";
+ public static final String SQL_PARSE_FAILED_ERROR_CODE = "SQL parse failed";
+ public static final String PLAN_VALIDATION_FAILED_ERROR_CODE = "Plan
validation failed";
+ public static final String SQL_QUERY_UNSUPPORTED_ERROR_CODE = "SQL query is
unsupported";
+
+ public enum FailType
+ {
+ UNKNOWN(500),
+ SERVER_ERROR(500),
+ CANCELED(500),
+ TIMEOUT(504),
+ CAPACITY_EXCEEDED(429),
+ QUERY_RUNTIME_FAILURE(501),
+ USER_ERROR(400),
+ UNSUPPORTED(501);
+
+ private final int expectedStatus;
+
+ FailType(int expectedStatus)
+ {
+ this.expectedStatus = expectedStatus;
+ }
+
+ public int getExpectedStatus()
+ {
+ return expectedStatus;
+ }
+ }
+
+ public static FailType fromErrorCode(String errorCode)
+ {
+ if (errorCode == null) {
+ return FailType.UNKNOWN;
+ }
+
+ switch (errorCode) {
+ case QUERY_CANCELED_ERROR_CODE:
+ return FailType.CANCELED;
+
+ // These error codes are generally expected to come from a
QueryInterruptedException
+ case QUERY_INTERRUPTED_ERROR_CODE:
+ case UNSUPPORTED_OPERATION_ERROR_CODE:
+ case UNKNOWN_EXCEPTION_ERROR_CODE:
+ case UNAUTHORIZED_ERROR_CODE:
Review Comment:
Tests don't seem to have changed. Should we start returning 401 anyway as
that would be the preferred behaviour?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
