imply-cheddar commented on code in PR #13564:
URL: https://github.com/apache/druid/pull/13564#discussion_r1050357689
##########
sql/src/main/java/org/apache/druid/sql/http/SqlResource.java:
##########
@@ -320,4 +181,210 @@ public Response cancelQuery(
return Response.status(Status.FORBIDDEN).build();
}
}
+
+ /**
+ * The SqlResource only generates metrics and doesn't keep track of
aggregate counts of successful/failed/interrupted
+ * queries, so this implementation is effectively just a noop.
+ */
+ private static class SqlResourceQueryMetricCounter implements
QueryResource.QueryMetricCounter
+ {
+ @Override
+ public void incrementSuccess()
+ {
+
+ }
+
+ @Override
+ public void incrementFailed()
+ {
+
+ }
+
+ @Override
+ public void incrementInterrupted()
+ {
+
+ }
+
+ @Override
+ public void incrementTimedOut()
+ {
+
+ }
+ }
+
+ private class SqlResourceQueryResultPusher extends QueryResultPusher
+ {
+ private final String sqlQueryId;
+ private final HttpStatement stmt;
+ private final SqlQuery sqlQuery;
+
+ public SqlResourceQueryResultPusher(
+ AsyncContext asyncContext,
+ String sqlQueryId,
+ HttpStatement stmt,
+ SqlQuery sqlQuery
+ )
+ {
+ super(
+ (HttpServletResponse) asyncContext.getResponse(),
+ SqlResource.this.jsonMapper,
+ SqlResource.this.responseContextConfig,
+ SqlResource.this.selfNode,
+ SqlResource.QUERY_METRIC_COUNTER,
+ sqlQueryId,
+ MediaType.APPLICATION_JSON_TYPE
+ );
+ this.sqlQueryId = sqlQueryId;
+ this.stmt = stmt;
+ this.sqlQuery = sqlQuery;
+ }
+
+ @Override
+ public ResultsWriter start()
+ {
+ return new ResultsWriter()
+ {
+ private ResultSet thePlan;
+
+ @Override
+ @Nullable
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ public QueryResponse<Object> start(HttpServletResponse response)
+ {
+ response.setHeader(SQL_QUERY_ID_RESPONSE_HEADER, sqlQueryId);
+
+ final QueryResponse<Object[]> retVal;
+ try {
+ thePlan = stmt.plan();
+ retVal = thePlan.run();
+ }
+ catch (RelOptPlanner.CannotPlanException e) {
+ throw new SqlPlanningException(
+ SqlPlanningException.PlanningError.UNSUPPORTED_SQL_ERROR,
+ e.getMessage()
+ );
+ }
+ // There is a claim that Calcite sometimes throws a
java.lang.AssertionError, but we do not have a test that can
+ // reproduce it checked into the code (the best we have is something
that uses mocks to throw an Error, which is
+ // dubious at best). We keep this just in case, but it might be
best to remove it and see where the
+ // AssertionErrors are coming from and do something to ensure that
they don't actually make it out of Calcite
+ catch (AssertionError e) {
+ log.warn(e, "AssertionError killed query: %s", sqlQuery);
+
+ // We wrap the exception here so that we get the sanitization.
java.lang.AssertionError apparently
+ // doesn't implement
org.apache.druid.common.exception.SanitizableException.
+ final QueryInterruptedException wrappedEx = new
QueryInterruptedException(e);
+ recordFailure(wrappedEx);
Review Comment:
For better or worse, this exception is actually returning a 500 instead of a
501 (which is what would happen for an error from an exception that we don't
know about). That said, I don't really know where the whole 501 thing came
from and I still don't believe that this exception even gets thrown, so I'll
change it to make it more consistent anyway.
--
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]
