gianm commented on a change in pull request #6302: Add SQL id, request logs,
and metrics
URL: https://github.com/apache/incubator-druid/pull/6302#discussion_r246161506
##########
File path: sql/src/main/java/org/apache/druid/sql/http/SqlResource.java
##########
@@ -148,12 +159,20 @@ public void write(final OutputStream outputStream)
throws IOException, WebApplic
writer.writeResponseEnd();
}
+ catch (Exception ex) {
+ e = ex;
+ log.error(ex, "Unable to send sql response [%s]",
sqlQueryId);
+ throw Throwables.propagate(ex);
+ }
finally {
yielder.close();
+ lifecycle.emitLogsAndMetrics(e, remoteAddr,
os.getCount());
}
}
}
)
+ .header("X-Druid-SQL-Query-Id", sqlQueryId)
+ .header("X-Druid-Native-Query-Ids",
joiner.join(plannerContext.getNativeQueryIds()))
Review comment:
I think the idea behind this `X-Druid-Native-Query-Ids` header is flawed. It
is not guaranteed that we'll have issued every native query by the time we
_start_ sending down SQL results. We can include the native IDs in the request
log (since we should have issued them all by then) but I don't think we can do
it in an HTTP response header.
For example, consider the native query ids for a SQL query that uses `UNION
ALL`. We start sending down results from the first query before the second
query is issued.
So, we might need to remove the header.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
