You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/11/24 20:03:04 UTC
[GitHub] [pinot] Jackie-Jiang commented on a change in pull request #7823: return exception when unavailable segments on empty broker response
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-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org