ankitsultana commented on code in PR #16641:
URL: https://github.com/apache/pinot/pull/16641#discussion_r2308235995
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -320,14 +325,18 @@ public void processTimeSeriesQueryEngine(@Suspended
AsyncResponse asyncResponse,
try {
try (RequestScope requestContext =
Tracing.getTracer().createRequestScope()) {
String queryString = requestCtx.getQueryString();
- PinotBrokerTimeSeriesResponse response =
executeTimeSeriesQuery(language, queryString, Map.of(), requestContext,
+ TimeSeriesBlock timeSeriesBlock = executeTimeSeriesQuery(language,
queryString, Map.of(), requestContext,
makeHttpIdentity(requestCtx), httpHeaders);
+ PinotBrokerTimeSeriesResponse response =
PinotBrokerTimeSeriesResponse.fromTimeSeriesBlock(timeSeriesBlock);
if (response.getErrorType() != null &&
!response.getErrorType().isEmpty()) {
asyncResponse.resume(Response.serverError().entity(response).build());
return;
}
asyncResponse.resume(response);
}
+ } catch (QueryException e) {
+
asyncResponse.resume(Response.serverError().entity(PinotBrokerTimeSeriesResponse.fromException(e))
Review Comment:
That's different and I think not the right way to look at it (as in we are
now discussing what is expected vs unexpected which can mean different things
to different people). If you look at it broadly you are doing stuff like:
```
throw new QueryException(QueryErrorCode.INTERNAL, "Time series query
engine not enabled.");
```
This could be defined as an "expected query failure" but with status quo we
will return 5xx.
Since Time Series API is new, I'd say let's stick with what makes sense and
is simple to reason about: we always try to return a well-formed response with
HTTP 200.
Users should check the error/exception fields in the response as defined in
the API Spec. Non-200 will only be returned if there was an exception that
escaped our error-handling mechanisms. These could come for instance from
request pre-processors or post-processors. In such cases, clients will most
likely not see a well formed error and should proceed with caution.
--
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]
