abhishekrb19 commented on code in PR #18639:
URL: https://github.com/apache/druid/pull/18639#discussion_r2434173446
##########
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java:
##########
@@ -760,13 +760,17 @@ public void onComplete(Result result)
return;
}
- boolean success = result.isSucceeded();
+ final int statusCode = result.getResponse().getStatus();
+ boolean success = statusCode == Status.OK.getStatusCode();
if (success) {
successfulQueryCount.incrementAndGet();
} else {
failedQueryCount.incrementAndGet();
}
- emitQueryTime(requestTimeNs, success, sqlQueryId, queryId,
result.getFailure());
+
+ // As router is simply a proxy, we don't make an effort to construct the
error code from the exception ourselves.
+ // We rely on broker to set this for us if the error occurs upstream.
Review Comment:
I think this should be "downstream", since brokers are downstream of routers?
##########
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java:
##########
@@ -760,13 +760,17 @@ public void onComplete(Result result)
return;
}
- boolean success = result.isSucceeded();
Review Comment:
Why was this logic for success changed from `result.isSucceeded()` to `
result.getResponse().getStatus()`? I think this would change the semantics of
what counts as a successful query to strictly a query that's returning with a
200 OK code.
Also, is this related to the error code change?
##########
processing/src/main/java/org/apache/druid/query/QueryMetrics.java:
##########
@@ -246,7 +245,7 @@ public interface QueryMetrics<QueryType extends Query<?>>
* Translates the given query exception into the appropriate failure status
code.
* See {@link DruidMetrics#computeStatusCode}.
*/
- void code(@Nullable Throwable error);
Review Comment:
Javadoc needs an update since there's no exception passed in anymore
##########
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java:
##########
@@ -760,13 +760,17 @@ public void onComplete(Result result)
return;
}
- boolean success = result.isSucceeded();
+ final int statusCode = result.getResponse().getStatus();
Review Comment:
On the router, I suppose this is fine especially as
`result.getResponse().getStatus()` would effectively translate to the same
error code as the underlying exception, if it’s of type
`DruidException`/`QueryException`?
Not a blocker for this PR, but I wonder if there are e2e embedded query
tests, where we can validate that the metrics emitted from these different
services are similar.
--
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]
