abhishekagarwal87 commented on code in PR #16672:
URL: https://github.com/apache/druid/pull/16672#discussion_r1713213668
##########
server/src/main/java/org/apache/druid/server/QueryResultPusher.java:
##########
@@ -223,6 +242,10 @@ private Response handleDruidException(ResultsWriter
resultsWriter, DruidExceptio
// also throwing the exception body into the response to make it
easier for the client to choke if it manages
// to parse a meaningful object out, but that's potentially an API
change so we leave that as an exercise for
// the future.
+
+ if (includeTrailerHeader) {
+ trailerFields.put(QueryResource.ERROR_MESSAGE_TRAILER_HEADER,
e.getMessage());
Review Comment:
IMO we should have one trailer to indicate whether the response was complete
or not. And the error message can be populated in another trailer.
##########
docs/querying/query-context.md:
##########
@@ -66,6 +66,7 @@ See [SQL query context](sql-query-context.md) for other query
context parameters
|`debug`| `false` | Flag indicating whether to enable debugging outputs for
the query. When set to false, no additional logs will be produced (logs
produced will be entirely dependent on your logging level). When set to true,
the following addition logs will be produced:<br />- Log the stack trace of the
exception (if any) produced by the query |
|`setProcessingThreadNames`|`true`| Whether processing thread names will be
set to `queryType_dataSource_intervals` while processing a query. This aids in
interpreting thread dumps, and is on by default. Query overhead can be reduced
slightly by setting this to `false`. This has a tiny effect in most scenarios,
but can be meaningful in high-QPS, low-per-segment-processing-time scenarios. |
|`sqlPlannerBloat`|`1000`|Calcite parameter which controls whether to merge
two Project operators when inlining expressions causes complexity to increase.
Implemented as a workaround to exception `There are not enough rules to produce
a node with desired properties: convention=DRUID, sort=[]` thrown after
rejecting the merge of two projects.|
+|`includeTrailerHeader`|`false`|Whether the response can include trailer
headers, which are currently used to indicate any error messages in the case of
a chunked response, if any.|
Review Comment:
This flag is too broad and may be unnecessary. I say broad because we may
start adding trailers for other purposes too
(https://github.com/apache/druid/pull/12058). What situations would you want to
set it to `false` and not have it on by default? That will help me understand
if its required.
##########
server/src/main/java/org/apache/druid/server/QueryResultPusher.java:
##########
@@ -121,6 +128,11 @@ public Response push()
final Response.ResponseBuilder startResponse = resultsWriter.start();
if (startResponse != null) {
startResponse.header(QueryResource.QUERY_ID_RESPONSE_HEADER, queryId);
+
+ if (includeTrailerHeader) {
+ startResponse.header(HttpHeader.TRAILER.toString(),
QueryResource.ERROR_MESSAGE_TRAILER_HEADER);
Review Comment:
what is the purpose of having this "header?
--
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]
