Jackie-Jiang commented on a change in pull request #7823:
URL: https://github.com/apache/pinot/pull/7823#discussion_r756387656
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -456,10 +456,18 @@ private BrokerResponseNative handleSQLRequest(long
requestId, String query, Json
int numUnavailableSegments = unavailableSegments.size();
requestStatistics.setNumUnavailableSegments(numUnavailableSegments);
+ List<QueryProcessingException> exceptions = new ArrayList<>();
+ if (numUnavailableSegments > 0) {
+ exceptions.add(new
QueryProcessingException(QueryException.BROKER_SEGMENT_UNAVAILABLE_ERROR_CODE,
+ String.format("%d segments %s unavailable", numUnavailableSegments,
unavailableSegments)));
+ }
Review comment:
Here we can store a list of `ProcessingException` which is easier to
manage
```suggestion
List<ProcessingException> exceptions = new ArrayList<>();
if (numUnavailableSegments > 0) {
exceptions.add(QueryException.getException(QueryException.BROKER_SEGMENT_UNAVAILABLE_ERROR_CODE,
String.format("%d segments %s unavailable",
numUnavailableSegments, unavailableSegments)));
}
```
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -486,14 +494,18 @@ private BrokerResponseNative handleSQLRequest(long
requestId, String query, Json
String errorMessage = e.getMessage();
LOGGER.info("{} {}: {}", errorMessage, requestId, query);
_brokerMetrics.addMeteredTableValue(rawTableName,
BrokerMeter.REQUEST_TIMEOUT_BEFORE_SCATTERED_EXCEPTIONS, 1);
- return new
BrokerResponseNative(QueryException.getException(QueryException.BROKER_TIMEOUT_ERROR,
errorMessage));
+ BrokerResponseNative brokerResponse =
+ new
BrokerResponseNative(QueryException.getException(QueryException.BROKER_TIMEOUT_ERROR,
errorMessage));
+ brokerResponse.addToExceptions(exceptions);
+ return brokerResponse;
Review comment:
This can also be simplified:
```suggestion
exceptions.add(QueryException.getException(QueryException.BROKER_TIMEOUT_ERROR,
errorMessage));
return new BrokerResponseNative(exceptions);
```
##########
File path:
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -456,10 +456,18 @@ private BrokerResponseNative handleSQLRequest(long
requestId, String query, Json
int numUnavailableSegments = unavailableSegments.size();
requestStatistics.setNumUnavailableSegments(numUnavailableSegments);
+ List<QueryProcessingException> exceptions = new ArrayList<>();
+ if (numUnavailableSegments > 0) {
+ exceptions.add(new
QueryProcessingException(QueryException.BROKER_SEGMENT_UNAVAILABLE_ERROR_CODE,
+ String.format("%d segments %s unavailable", numUnavailableSegments,
unavailableSegments)));
+ }
+
if (offlineBrokerRequest == null && realtimeBrokerRequest == null) {
LOGGER.info("No server found for request {}: {}", requestId, query);
_brokerMetrics.addMeteredTableValue(rawTableName,
BrokerMeter.NO_SERVER_FOUND_EXCEPTIONS, 1);
- return BrokerResponseNative.EMPTY_RESULT;
+ BrokerResponseNative brokerResponse = BrokerResponseNative.EMPTY_RESULT;
+ brokerResponse.addToExceptions(exceptions);
+ return brokerResponse;
Review comment:
(major) We cannot modify the constant broker response. Here we need to
create a new one. If we apply the suggested change above, we can directly use
the exception constructor
```suggestion
return new BrokerResponseNative(exceptions);
```
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
