a2l007 commented on a change in pull request #10746:
URL: https://github.com/apache/druid/pull/10746#discussion_r556218910
##########
File path:
processing/src/main/java/org/apache/druid/query/ResourceLimitExceededException.java
##########
@@ -19,18 +19,33 @@
package org.apache.druid.query;
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.java.util.common.StringUtils;
/**
* Exception indicating that an operation failed because it exceeded some
configured resource limit.
*
- * This is used as a marker exception by {@link QueryInterruptedException} to
report the "Resource limit exceeded"
- * error code.
+ * This is a {@link BadQueryException} because it will likely a user's
misbehavior when this exception is thrown.
+ * Druid cluster operators set some set of resource limitations for some good
reason. When a user query requires
+ * too many resources, it will likely mean that the user query should be
modified to not use excessive resources.
Review comment:
If we change this, the exception type and code changes from RE to
ResourceLimitExceeded for queries exceeding maxScatterGatherBytes. What are the
side effects that could happen? Sorry if I'm missing something :)
##########
File path: server/src/main/java/org/apache/druid/server/QueryResource.java
##########
@@ -463,36 +481,36 @@ Response ok(Object object) throws IOException
Response gotError(Exception e) throws IOException
{
- return Response.serverError()
- .type(contentType)
- .entity(
- newOutputWriter(null, null, false)
-
.writeValueAsBytes(QueryInterruptedException.wrapIfNeeded(e))
- )
- .build();
+ return buildNonOkResponse(
+ Status.INTERNAL_SERVER_ERROR.getStatusCode(),
+ QueryInterruptedException.wrapIfNeeded(e)
+ );
}
Response gotTimeout(QueryTimeoutException e) throws IOException
{
- return Response.status(QueryTimeoutException.STATUS_CODE)
- .type(contentType)
- .entity(
- newOutputWriter(null, null, false)
- .writeValueAsBytes(e)
- )
- .build();
+ return buildNonOkResponse(QueryTimeoutException.STATUS_CODE, e);
}
Response gotLimited(QueryCapacityExceededException e) throws IOException
{
- return Response.status(QueryCapacityExceededException.STATUS_CODE)
- .entity(newOutputWriter(null, null,
false).writeValueAsBytes(e))
- .build();
+ return buildNonOkResponse(QueryCapacityExceededException.STATUS_CODE, e);
}
Response gotUnsupported(QueryUnsupportedException e) throws IOException
{
- return Response.status(QueryUnsupportedException.STATUS_CODE)
+ return buildNonOkResponse(QueryUnsupportedException.STATUS_CODE, e);
+ }
+
+ Response gotBadQuery(BadQueryException e) throws IOException
+ {
+ return buildNonOkResponse(BadQueryException.STATUS_CODE, e);
Review comment:
Thanks works fine now. I agree that we can avoid deeper tinkering of
QueryException before the release and it can be picked up post release.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org
