You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/01/12 08:41:15 UTC
[GitHub] [druid] jihoonson opened a new pull request #10746: Tidy up HTTP status codes for query errors
jihoonson opened a new pull request #10746:
URL: https://github.com/apache/druid/pull/10746
### Description
Druid querying system returns an HTTP status code on query failures. Currently, most of query failures return an internal error (500) which could be misleading in some cases. This PR is to tidy up this and return a proper status code. Here is a list of query errors, their current status codes, and proposed changes.
#### Query planning errors
Exception | Description| Current code | Proposed new code
------------ | ------------- | ------------- | -------------
SqlParseException and ValidationException from Calcite | Query planning failed | 500 | **400**
CannotPlanException from Calcite | Failed to find the best plan. Likely an internal error | 500 |
RelConversionException from Calcite | Conversion from SQL to Druid failed | 500 |
CannotBuildQueryException | Failed to create a Druid native query from DruidRel. Likely an internal error | 500 |
Others | Unknown errors | 500 |
#### Query execution errors
Exception | Description| Current code | Proposed new code
------------ | ------------- | ------------- | -------------
QueryTimeoutException | Query execution didn't finish in timeout | 504 |
Unauthorized exception | Authentication failed to execute the query | 401 |
ForbiddenException | Authorization failed to execute the query | 403 |
QueryCapacityExceededException | Query failed to schedule because of the lack of resources available at the time when the query was submitted | 429 |
ResourceLimitExceededException | Query asked more resources than configured threshold | 500 | **400**
InsufficientResourceException | Query failed to schedule because of lack of merge buffers available at the time when it was submitted | 500 | **429**, merged to QueryCapacityExceededException
QueryUnsupportedException | Unsupported functionality | 400 | **501**
Others | Unknown exceptions | 500 |
`BadQueryException` is added to represent all kinds of query exceptions that should return 400.
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
<hr>
##### Key changed/added classes in this PR
* `BadQueryException`
* `ResourceLimitExceededException`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10746:
URL: https://github.com/apache/druid/pull/10746#issuecomment-759859757
All tests passed except for the test coverage check mentioned above. I'm going to go ahead and merge this to cut the branch. @jon-wei @a2l007 any thoughts?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
a2l007 commented on pull request #10746:
URL: https://github.com/apache/druid/pull/10746#issuecomment-759482907
LGTM after CI.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10746:
URL: https://github.com/apache/druid/pull/10746#discussion_r556202316
##########
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 for testing! I thought I fixed all cases where exceptions are wrapped in `QueryInterruptedException`, but missed `JsonParserIterator`. My intention was to fix it in this PR, so I went ahead and fixed it. It turns out there is an issue of that the type of query exception is lost after JSON deserialization. I ended up with [this ugly switch clause](https://github.com/apache/druid/pull/10746/files#diff-28a786f3719500e89592ff9617a926736337b723e4245c410601669db0f29ee5R207-R259) to restore the exception type. A better approach is modifying `QueryException` to store the type using Jackson, but I wasn't 100% sure what is the side effect there too. The ugly switch clause seems at least safer than modifying `QueryException`. I think we can rework the serde system of `QueryException` after release. What do you think?
##########
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:
Good catch :slightly_smiling_face: It should be, but I wasn't 100% sure whether it could cause any side effects if I did it. I left a comment about it [here](https://github.com/apache/druid/pull/10746/files#diff-28a786f3719500e89592ff9617a926736337b723e4245c410601669db0f29ee5R171). I will also make an issue to track it.
##########
File path: processing/src/main/java/org/apache/druid/query/BadQueryException.java
##########
@@ -20,12 +20,16 @@
package org.apache.druid.query;
/**
- * This exception is thrown when the requested operation cannot be completed due to a lack of available resources.
+ * An abstract class for all query exceptions that should return a bad request status code (404).
Review comment:
Oops, yes it should be.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] suneet-s commented on pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10746:
URL: https://github.com/apache/druid/pull/10746#issuecomment-759526680
> Will update docs as a follow-up.
As part of this follow-up PR can you also add test cases to the query IT. I think the scenarios you outlined in the PR description are great and should all have an IT associated with it.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a change in pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jon-wei commented on a change in pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #10746:
URL: https://github.com/apache/druid/pull/10746#discussion_r556245770
##########
File path: processing/src/main/java/org/apache/druid/query/QueryUnsupportedException.java
##########
@@ -24,21 +24,20 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import javax.annotation.Nullable;
-import java.net.InetAddress;
/**
* This exception is for the query engine to surface when a query cannot be run. This can be due to the
* following reasons: 1) The query is not supported yet. 2) The query is not something Druid would ever supports.
* For these cases, the exact causes and details should also be documented in Druid user facing documents.
*
- * As a {@link QueryException} it is expected to be serialied to a json response, but will be mapped to
- * {@link #STATUS_CODE} instead of the default HTTP 500 status.
+ * As a {@link QueryException} it is expected to be serialied to a json response with a proper HTTP error code
Review comment:
serialied -> serialized
##########
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:
Suggest rewording to something like:
"This is a {@link BadQueryException} because it likely indicates a user's misbehavior when this exception is thrown. The resource limitations set by Druid cluster operators are typically less flexible than the parameters of a user query, so when a user query requires too many resources, the likely remedy is that the user query should be modified to use fewer resources, or to reduce query volume."
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #10746:
URL: https://github.com/apache/druid/pull/10746
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10746:
URL: https://github.com/apache/druid/pull/10746#issuecomment-758500465
Will update docs as a follow-up.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a change in pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10746:
URL: https://github.com/apache/druid/pull/10746#discussion_r556043383
##########
File path: processing/src/main/java/org/apache/druid/query/BadQueryException.java
##########
@@ -20,12 +20,16 @@
package org.apache.druid.query;
/**
- * This exception is thrown when the requested operation cannot be completed due to a lack of available resources.
+ * An abstract class for all query exceptions that should return a bad request status code (404).
Review comment:
Shouldn't it be 400 here?
##########
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:
Based on the description, shouldn't [this exception](https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/client/DirectDruidClient.java#L447) also be categorized as ResourceLimitExceeded ?
##########
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:
I was curious whether this would catch ResourceLimitExceeded exceptions wrapped under `QueryInterruptedException` and so I ran the PR on my test cluster. It looks like such tests are still giving us 500 instead of 400. Related stack trace:
```
HTTP/1.1 500 Internal Server Error
< Content-Type: application/json
< Content-Length: 372
{
"error" : "Resource limit exceeded",
"trace":
org.apache.druid.query.QueryInterruptedException: Not enough aggregation buffer space to execute this query. Try increasing druid.processing.buffer.sizeBytes or enable disk spilling by setting druid.query.groupBy.maxOnDiskStorage to a positive number.
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) ~[?:1.8.0_231]
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62) ~[?:1.8.0_231]
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) ~[?:1.8.0_231]
at java.lang.reflect.Constructor.newInstance(Constructor.java:423) ~[?:1.8.0_231]
at com.fasterxml.jackson.databind.introspect.AnnotatedConstructor.call(AnnotatedConstructor.java:124) ~[jackson-databind-2.10.5.1.jar:2.10.5.1]
at com.fasterxml.jackson.databind.deser.std.StdValueInstantiator.createFromObjectWith(StdValueInstantiator.java:283) ~[jackson-databind-2.10.5.1.jar:2.10.5.1]
at com.fasterxml.jackson.databind.deser.ValueInstantiator.createFromObjectWith(ValueInstantiator.java:229) ~[jackson-databind-2.10.5.1.jar:2.10.5.1]
at com.fasterxml.jackson.databind.deser.impl.PropertyBasedCreator.build(PropertyBasedCreator.java:198) ~[jackson-databind-2.10.5.1.jar:2.10.5.1]
at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:422) ~[jackson-databind-2.10.5.1.jar:2.10.5.1]
at com.fasterxml.jackson.databind.deser.std.ThrowableDeserializer.deserializeFromObject(ThrowableDeserializer.java:65) ~[jackson-databind-2.10.5.1.jar:2.10.5.1]
at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:159) ~[jackson-databind-2.10.5.1.jar:2.10.5.1]
at com.fasterxml.jackson.databind.ObjectMapper._readValue(ObjectMapper.java:4189) ~[jackson-databind-2.10.5.1.jar:2.10.5.1]
at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:2476) ~[jackson-databind-2.10.5.1.jar:2.10.5.1]
at org.apache.druid.client.JsonParserIterator.init(JsonParserIterator.java:182) [classes/:?]
at org.apache.druid.client.JsonParserIterator.hasNext(JsonParserIterator.java:90) [classes/:?]
```
Maybe as a follow-up we could check the wrapped error code while handling QueryInterruptedException to determine the correct exception type.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10746:
URL: https://github.com/apache/druid/pull/10746#issuecomment-759843464
```Diff coverage statistics:
------------------------------------------------------------------------------
| lines | branches | functions | path
------------------------------------------------------------------------------
| 100% (17/17) | 100% (0/0) | 92% (12/13) | org/apache/druid/server/QueryResource.java
| 32% (11/34) | 44% (4/9) | 71% (27/38) | org/apache/druid/client/JsonParserIterator.java
| 0% (0/1) | 100% (0/0) | 100% (1/1) | org/apache/druid/client/DirectDruidClient.java
| 100% (3/3) | 100% (0/0) | 50% (2/4) | org/apache/druid/server/security/SecuritySanityCheckFilter.java
| 100% (1/1) | 100% (0/0) | 66% (6/9) | org/apache/druid/server/security/ForbiddenException.java
------------------------------------------------------------------------------
Total diff coverage:
- lines: 57% (32/56)
- branches: 44% (4/9)
- functions: 73% (48/65)
ERROR: Insufficient branch coverage of 44% (4/9). Required 50%.
```
The test coverage check failed. I agree those branches in `JsonParserIterator` should be covered, but since this is blocking a branch cut, I will add them as a follow-up. Other integration test failures seem flaky. I just restarted them.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10746:
URL: https://github.com/apache/druid/pull/10746#discussion_r556270879
##########
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:
@a2l007 oh, you are correct about the exception type in `checkTotalBytesLimit()`. My point was that this exception doesn't seem to be propagated to `JsonParserIterator` anyway because [`NettyHttpClient` doesn't do it](https://github.com/apache/druid/blob/master/core/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java#L263). I think `NettyHttpClient` should call `retVal.setException()` instead of `retVal.set(null)`, but I haven't studied the code enough around it to figure out why we are doing this. Anyway, it doesn't look harm to change the exception type in this PR, so I changed :slightly_smiling_face:
@jon-wei :+1: Updated the javadoc.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10746:
URL: https://github.com/apache/druid/pull/10746#issuecomment-759713951
> > Will update docs as a follow-up.
>
> As part of this follow-up PR can you also add test cases to the query IT. I think the scenarios you outlined in the PR description are great and should all have an IT associated with it.
Good idea. We need integration tests for this.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
a2l007 commented on pull request #10746:
URL: https://github.com/apache/druid/pull/10746#issuecomment-759861032
SGTM. I've gone ahead and created a issue #10756 for this so that we can keep track.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #10746:
URL: https://github.com/apache/druid/pull/10746#issuecomment-759861503
@a2l007 thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a change in pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10746:
URL: https://github.com/apache/druid/pull/10746#discussion_r556556189
##########
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:
@jihoonson Makes sense. Thanks for clarifying!
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #10746: Tidy up HTTP status codes for query errors
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10746:
URL: https://github.com/apache/druid/pull/10746#discussion_r556271084
##########
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!
##########
File path: processing/src/main/java/org/apache/druid/query/QueryUnsupportedException.java
##########
@@ -24,21 +24,20 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import javax.annotation.Nullable;
-import java.net.InetAddress;
/**
* This exception is for the query engine to surface when a query cannot be run. This can be due to the
* following reasons: 1) The query is not supported yet. 2) The query is not something Druid would ever supports.
* For these cases, the exact causes and details should also be documented in Druid user facing documents.
*
- * As a {@link QueryException} it is expected to be serialied to a json response, but will be mapped to
- * {@link #STATUS_CODE} instead of the default HTTP 500 status.
+ * As a {@link QueryException} it is expected to be serialied to a json response with a proper HTTP error code
Review comment:
Oops, thanks.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org