kfaraz commented on code in PR #13609:
URL: https://github.com/apache/druid/pull/13609#discussion_r1055103233
##########
sql/src/main/java/org/apache/druid/sql/http/SqlResource.java:
##########
@@ -121,22 +121,8 @@ public Response doPost(
try {
Thread.currentThread().setName(StringUtils.format("sql[%s]",
sqlQueryId));
- // We use an async context not because we are actually going to run this
async, but because we want to delay
- // the decision of what the response code should be until we have gotten
the first few data points to return.
- // Returning a Response object from this point forward requires that
object to know the status code, which we
- // don't actually know until we are in the accumulator, but if we try to
return a Response object from the
- // accumulator, we cannot properly stream results back, because the
accumulator won't release control of the
- // Response until it has consumed the underlying Sequence.
- final AsyncContext asyncContext = req.startAsync();
-
- try {
- QueryResultPusher pusher = new
SqlResourceQueryResultPusher(asyncContext, sqlQueryId, stmt, sqlQuery);
Review Comment:
Nit: the `sqlQueryId` variable can be removed now.
##########
server/src/main/java/org/apache/druid/server/QueryResource.java:
##########
@@ -561,20 +549,27 @@ public ResultsWriter start()
{
return new ResultsWriter()
{
+ private QueryResponse<Object> queryResponse;
+
@Override
- public QueryResponse<Object> start(HttpServletResponse response)
+ public Response.ResponseBuilder start()
{
- final QueryResponse<Object> queryResponse = queryLifecycle.execute();
+ queryResponse = queryLifecycle.execute();
final ResponseContext responseContext =
queryResponse.getResponseContext();
final String prevEtag = getPreviousEtag(req);
if (prevEtag != null &&
prevEtag.equals(responseContext.getEntityTag())) {
queryLifecycle.emitLogsAndMetrics(null, req.getRemoteAddr(), -1);
counter.incrementSuccess();
- response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
- return null;
+ return Response.status(Status.NOT_MODIFIED);
Review Comment:
IIUC, this is the only case in which `ResultsWriter.start()` would return a
non-null value. I wonder if this method could just return a boolean and we
handle it correctly in `ResultPusher.push()`.
##########
server/src/main/java/org/apache/druid/server/QueryResultPusher.java:
##########
@@ -159,10 +187,15 @@ public void push()
log.warn(e, "Suppressing exception closing accumulator for
query[%s]", queryId);
}
}
+ if (asyncContext != null) {
+ asyncContext.complete();
+ }
}
+ return null;
Review Comment:
Style: maybe put this right after the line with
`resultsWriter.recordSuccess()` for readability.
##########
server/src/main/java/org/apache/druid/server/QueryResource.java:
##########
@@ -561,20 +549,27 @@ public ResultsWriter start()
{
return new ResultsWriter()
{
+ private QueryResponse<Object> queryResponse;
+
@Override
- public QueryResponse<Object> start(HttpServletResponse response)
+ public Response.ResponseBuilder start()
{
- final QueryResponse<Object> queryResponse = queryLifecycle.execute();
+ queryResponse = queryLifecycle.execute();
final ResponseContext responseContext =
queryResponse.getResponseContext();
final String prevEtag = getPreviousEtag(req);
if (prevEtag != null &&
prevEtag.equals(responseContext.getEntityTag())) {
queryLifecycle.emitLogsAndMetrics(null, req.getRemoteAddr(), -1);
counter.incrementSuccess();
- response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
- return null;
+ return Response.status(Status.NOT_MODIFIED);
Review Comment:
We could even choose to return a single object containing the boolean and
the (nullable) `queryResponse` rather than exposing `getQueryResponse()` and
relying on it being called after `start()`.
--
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]
