ankitsultana commented on code in PR #16641:
URL: https://github.com/apache/pinot/pull/16641#discussion_r2304417428
##########
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotClientRequest.java:
##########
@@ -295,10 +298,12 @@ public void processTimeSeriesQueryEngine(Map<String,
String> queryParams, @Suspe
String language = queryParams.get(Request.LANGUAGE);
String queryString = queryParams.get(Request.QUERY);
try (RequestScope requestContext =
Tracing.getTracer().createRequestScope()) {
- PinotBrokerTimeSeriesResponse response =
executeTimeSeriesQuery(language, queryString, queryParams,
+ TimeSeriesBlock timeSeriesBlock = executeTimeSeriesQuery(language,
queryString, queryParams,
requestContext, makeHttpIdentity(requestCtx), httpHeaders);
- asyncResponse.resume(response.toBrokerResponse());
+
asyncResponse.resume(TimeSeriesResponseMapper.toBrokerResponse(timeSeriesBlock));
}
+ } catch (QueryException e) {
+ asyncResponse.resume(TimeSeriesResponseMapper.toBrokerResponse(e));
} catch (Exception e) {
LOGGER.error("Caught exception while processing POST timeseries
request", e);
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.UNCAUGHT_POST_EXCEPTIONS, 1L);
Review Comment:
not introduced in this PR: can you also ensure that we don't throw a
WebApplicationException and instead throw a proper time series response with
error message for the uncaught case too?
##########
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:
Pinot brokers generally return 200 even for query failures. I think it's
best to stick with that here too. Using Response.serverError will likely return
a 5xx afaik.
Same for the uncaught case below.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/TimeSeriesRequestHandler.java:
##########
@@ -121,31 +123,31 @@ public PinotBrokerTimeSeriesResponse
handleTimeSeriesRequest(String lang, String
try {
timeSeriesRequest = buildRangeTimeSeriesRequest(lang,
rawQueryParamString, queryParams);
} catch (URISyntaxException e) {
- return PinotBrokerTimeSeriesResponse.newErrorResponse("BAD_REQUEST",
"Error building RangeTimeSeriesRequest");
+ throw new QueryException(QueryErrorCode.TIMESERIES_PARSING, "Error
building RangeTimeSeriesRequest", e);
}
TimeSeriesLogicalPlanResult logicalPlanResult =
_queryEnvironment.buildLogicalPlan(timeSeriesRequest);
// If there are no buckets in the logical plan, return an empty response.
if (logicalPlanResult.getTimeBuckets().getNumBuckets() == 0) {
- return PinotBrokerTimeSeriesResponse.newEmptyResponse();
+ return new TimeSeriesBlock(logicalPlanResult.getTimeBuckets(), new
HashMap<>());
}
TimeSeriesDispatchablePlan dispatchablePlan =
_queryEnvironment.buildPhysicalPlan(timeSeriesRequest,
requestContext, logicalPlanResult);
tableLevelAccessControlCheck(httpHeaders,
dispatchablePlan.getTableNames());
- timeSeriesResponse = _queryDispatcher.submitAndGet(requestContext,
dispatchablePlan,
- timeSeriesRequest.getTimeout().toMillis(), new HashMap<>());
- return timeSeriesResponse;
+ timeSeriesBlock =
_queryDispatcher.submitAndGet(requestContext.getRequestId(), dispatchablePlan,
+ timeSeriesRequest.getTimeout().toMillis(), new HashMap<>(),
requestContext);
+ return timeSeriesBlock;
+ } catch (Exception e) {
+
_brokerMetrics.addMeteredGlobalValue(BrokerMeter.TIME_SERIES_GLOBAL_QUERIES_FAILED,
1);
+ LOGGER.warn("time-series query failed with error: {}", e.getMessage());
Review Comment:
can you remove this log? This was a TODO in the older code
--
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]
