yashmayya commented on code in PR #15037:
URL: https://github.com/apache/pinot/pull/15037#discussion_r1975177226
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -368,9 +364,10 @@ protected BrokerResponse handleRequest(long requestId,
String query, SqlNodeAndO
Set<String> unavailableSegments = entry.getValue();
int unavailableSegmentsInSubPlan = unavailableSegments.size();
numUnavailableSegments += unavailableSegmentsInSubPlan;
-
brokerResponse.addException(QueryException.getException(QueryException.SERVER_SEGMENT_MISSING_ERROR,
- String.format("Found %d unavailable segments for table %s: %s",
unavailableSegmentsInSubPlan, tableName,
- toSizeLimitedString(unavailableSegments,
NUM_UNAVAILABLE_SEGMENTS_TO_LOG))));
+ BrokerResponseErrorMessage errMsg = new
BrokerResponseErrorMessage(QueryErrorCode.SERVER_SEGMENT_MISSING,
+ "Found " + unavailableSegmentsInSubPlan + " unavailable segments
for table " + tableName + ": "
+ + toSizeLimitedString(unavailableSegments,
NUM_UNAVAILABLE_SEGMENTS_TO_LOG));
+ brokerResponse.addException(errMsg);
Review Comment:
Same comment as above that it seems pretty odd to add an error code / error
message string in a method called `addException` but we can do that refactoring
later if you'd prefer.
##########
pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTable.java:
##########
@@ -36,10 +37,15 @@
*/
public interface DataTable {
+ @Deprecated
void addException(ProcessingException processingException);
void addException(int exceptionCode, String exceptionMsg);
+ default void addException(QueryErrorCode exceptionCode, String exceptionMsg)
{
Review Comment:
This is only being used in tests currently?
##########
pinot-spi/src/main/java/org/apache/pinot/spi/exception/QueryException.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.exception;
+
+/**
+ * The base exception for query processing in Pinot.
+ *
+ * This exception is captured by the query engine and converted to a
user-friendly message in a standardized format
+ * (ie converted into a JSON payload in an HTTP response).
+ *
+ * Contrary to {@code org.apache.pinot.common.response.ProcessingException},
which is usually designed to be thrown by
+ * {@code org.apache.pinot.common.exception.QueryException} in a
non-allocating, non-throwing and stack-trace-less
Review Comment:
Aren't we removing `org.apache.pinot.common.exception.QueryException` in
this PR?
##########
pinot-common/src/main/java/org/apache/pinot/common/function/FunctionInvoker.java:
##########
@@ -112,7 +113,11 @@ public void convertTypes(Object[] arguments) {
PinotDataType argumentType =
FunctionUtils.getArgumentType(argumentClass);
Preconditions.checkArgument(parameterType != null && argumentType !=
null,
"Cannot convert value from class: %s to class: %s", argumentClass,
parameterClass);
- arguments[i] = parameterType.convert(argument, argumentType);
+ try {
+ arguments[i] = parameterType.convert(argument, argumentType);
+ } catch (Exception e) {
+ throw QueryErrorCode.QUERY_EXECUTION.asException("Invalid conversion:
" + e.getMessage(), e);
Review Comment:
We should probably include the method name and arguments for additional
context?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseCombineOperator.java:
##########
@@ -157,10 +156,12 @@ protected void stopProcess() {
}
protected ExceptionResultsBlock getTimeoutResultsBlock(int numBlocksMerged) {
- LOGGER.error("Timed out while polling results block, numBlocksMerged: {}
(query: {})", numBlocksMerged,
- _queryContext);
- return new ExceptionResultsBlock(QueryException.EXECUTION_TIMEOUT_ERROR,
- new TimeoutException("Timed out while polling results block"));
+ String logMsg = "Timed out while polling results block, numBlocksMerged: "
+ numBlocksMerged + " (query: "
+ + _queryContext + ")";
+ LOGGER.error(logMsg);
+ QueryErrorCode errCode = QueryErrorCode.EXECUTION_TIMEOUT;
+ QueryErrorMessage errMSg = new QueryErrorMessage(errCode, "Timed out while
polling results block", logMsg);
Review Comment:
```suggestion
QueryErrorMessage errMsg = new QueryErrorMessage(errCode, "Timed out
while polling results block", logMsg);
```
nit
##########
pinot-spi/src/main/java/org/apache/pinot/spi/exception/EarlyTerminationException.java:
##########
@@ -22,17 +22,17 @@
* The {@code EarlyTerminationException} can be thrown from
{Operator#nextBlock()} when the operator is early
* terminated (interrupted).
*/
-public class EarlyTerminationException extends RuntimeException {
+public class EarlyTerminationException extends QueryException {
public EarlyTerminationException() {
- super();
+ super(QueryErrorCode.INTERNAL);
Review Comment:
Could we consider adding a separate error code for this? Also a side note, I
see this early termination exception being used a little inconsistently - for
instance, `BaseStreamingCombineOperator` uses it for timeouts as well as
interruptions.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/streaming/StreamingInstanceResponseOperator.java:
##########
@@ -89,11 +89,12 @@ protected InstanceResponseBlock getNextBlock() {
}
} catch (EarlyTerminationException e) {
Exception killedErrorMsg =
Tracing.getThreadAccountant().getErrorStatus();
- return new InstanceResponseBlock(new ExceptionResultsBlock(new
QueryCancelledException(
- "Cancelled while streaming results" + (killedErrorMsg == null ?
StringUtils.EMPTY : " " + killedErrorMsg),
- e)));
+ QueryErrorMessage errMsg =
QueryErrorMessage.safeMsg(QueryErrorCode.QUERY_CANCELLATION,
+ "Cancelled while streaming results" + (killedErrorMsg == null ?
StringUtils.EMPTY : " " + killedErrorMsg));
+ return new InstanceResponseBlock(new ExceptionResultsBlock(errMsg));
} catch (Exception e) {
- return new InstanceResponseBlock(new
ExceptionResultsBlock(QueryException.INTERNAL_ERROR, e));
+ QueryErrorMessage errMsg =
QueryErrorMessage.safeMsg(QueryErrorCode.INTERNAL, e.getMessage());
+ return new InstanceResponseBlock(new ExceptionResultsBlock(errMsg));
Review Comment:
Shouldn't we log the exception in both these branches?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -544,6 +548,19 @@ public static QueryResult runReducer(long requestId,
System.currentTimeMillis() - startTimeMs);
}
+ // TODO: Improve the way the errors are compared
+ private static int compareErrors(Map.Entry<Integer, String> entry1,
Map.Entry<Integer, String> entry2) {
+ int errorCode1 = entry1.getKey();
+ int errorCode2 = entry2.getKey();
+ if (errorCode1 == QueryErrorCode.QUERY_VALIDATION.getId()) {
+ return errorCode1;
+ }
+ if (errorCode2 == QueryErrorCode.QUERY_VALIDATION.getId()) {
+ return errorCode2;
+ }
+ return Integer.compare(errorCode1, errorCode2);
+ }
Review Comment:
I don't follow this - what's special about `QUERY_VALIDATION` errors?
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/DataBlock.java:
##########
@@ -41,10 +42,15 @@ public interface DataBlock {
int getNumberOfColumns();
+ @Deprecated
Review Comment:
nit: can we add a Javadoc pointing to the recommended alternative here?
##########
pinot-common/src/main/java/org/apache/pinot/common/response/BrokerResponse.java:
##########
@@ -76,11 +76,11 @@ default void toOutputStream(OutputStream outputStream)
/**
* Returns the processing exceptions encountered during the query execution.
*/
- List<QueryProcessingException> getExceptions();
+ List<BrokerResponseErrorMessage> getExceptions();
Review Comment:
nit: we should probably rename the method as well? Similarly for
`getProcessingExceptions` and other methods in `BrokerResponseNative` since
these aren't real exceptions as such. Okay to do in a follow up though since
this PR already has a ton of changes.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -222,16 +222,16 @@ protected BrokerResponse handleRequest(long requestId,
String query, SqlNodeAndO
() -> finalQueryEnvironment.planQuery(query,
sqlNodeAndOptions, requestId),
queryTimer.getRemainingTime());
} catch (TimeoutException | InterruptedException e) {
-
requestContext.setErrorCode(QueryException.BROKER_TIMEOUT_ERROR_CODE);
- return new
BrokerResponseNative(QueryException.BROKER_TIMEOUT_ERROR);
+ requestContext.setErrorCode(QueryErrorCode.BROKER_TIMEOUT);
+ return new BrokerResponseNative(QueryErrorCode.BROKER_TIMEOUT,
"BrokerTimeoutError");
Review Comment:
Might be worth adding here that the timeout was during query compilation /
planning?
##########
pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerResponseNative.java:
##########
@@ -59,16 +59,16 @@
public class BrokerResponseNative implements BrokerResponse {
public static final BrokerResponseNative EMPTY_RESULT =
BrokerResponseNative.empty();
public static final BrokerResponseNative NO_TABLE_RESULT =
- new BrokerResponseNative(QueryException.BROKER_RESOURCE_MISSING_ERROR);
+ new BrokerResponseNative(QueryErrorCode.BROKER_RESOURCE_MISSING,
"BrokerResourceMissingError");
Review Comment:
Let's use the `defaultMessage` from the enum instead of hardcoding these
constants in multiple places?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -726,18 +722,18 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
String realtimeRoutingPolicy = realtimeBrokerRequest != null ?
getRoutingPolicy(realtimeTableConfig) : null;
String offlineRoutingPolicy = offlineBrokerRequest != null ?
getRoutingPolicy(offlineTableConfig) : null;
errorMessage = addRoutingPolicyInErrMsg(errorMessage,
realtimeRoutingPolicy, offlineRoutingPolicy);
-
exceptions.add(QueryException.getException(QueryException.BROKER_SEGMENT_UNAVAILABLE_ERROR,
errorMessage));
+ errorMsgs.add(new
BrokerResponseErrorMessage(QueryErrorCode.BROKER_SEGMENT_UNAVAILABLE,
errorMessage));
_brokerMetrics.addMeteredTableValue(rawTableName,
BrokerMeter.BROKER_RESPONSES_WITH_UNAVAILABLE_SEGMENTS, 1);
}
if (offlineBrokerRequest == null && realtimeBrokerRequest == null) {
- if (!exceptions.isEmpty()) {
- ProcessingException firstException = exceptions.get(0);
- String logTail = exceptions.size() > 1 ? (exceptions.size()) + "
exceptions found. Logging only the first one"
+ if (!errorMsgs.isEmpty()) {
+ BrokerResponseErrorMessage firstErrorMsg = errorMsgs.get(0);
+ String logTail = errorMsgs.size() > 1 ? (errorMsgs.size()) + "
errorMsgs found. Logging only the first one"
: "1 exception found";
- LOGGER.info("No server found for request {}: {}. {}", requestId,
query, logTail, firstException);
+ LOGGER.info("No server found for request {}: {}. {}", requestId,
query, logTail, firstErrorMsg);
Review Comment:
There's three placeholders here and 4 arguments (the 4th is no longer an
instance of `Throwable`).
##########
pinot-spi/src/main/java/org/apache/pinot/spi/exception/QueryErrorCode.java:
##########
@@ -0,0 +1,141 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.exception;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public enum QueryErrorCode {
+ JSON_PARSING(100, "JsonParsingError"),
+ SQL_PARSING(150, "SQLParsingError"),
+ SQL_RUNTIME(160, "SQLRuntimeError"),
+ ACCESS_DENIED(180, "AccessDenied"),
+ TABLE_DOES_NOT_EXIST(190, "TableDoesNotExistError"),
+ TABLE_IS_DISABLED(191, "TableIsDisabledError"),
+ QUERY_EXECUTION(200, "QueryExecutionError"),
+ SERVER_SHUTTING_DOWN(210, "ServerShuttingDown"),
+ SERVER_OUT_OF_CAPACITY(211, "ServerOutOfCapacity"),
+ SERVER_TABLE_MISSING(230, "ServerTableMissing"),
+ SERVER_SEGMENT_MISSING(235, "ServerSegmentMissing"),
+ QUERY_SCHEDULING_TIMEOUT(240, "QuerySchedulingTimeoutError"),
+ SERVER_RESOURCE_LIMIT_EXCEEDED(245, "ServerResourceLimitExceededError"),
+ EXECUTION_TIMEOUT(250, "ExecutionTimeoutError"),
+ BROKER_SEGMENT_UNAVAILABLE(305, ""),
+ BROKER_TIMEOUT(400, "BrokerTimeoutError"),
+ BROKER_RESOURCE_MISSING(410, "BrokerResourceMissingError"),
+ BROKER_INSTANCE_MISSING(420, "BrokerInstanceMissingError"),
+ BROKER_REQUEST_SEND(425, "BrokerRequestSend"),
+ SERVER_NOT_RESPONDING(427, "ServerNotResponding"),
+ TOO_MANY_REQUESTS(429, "TooManyRequests"),
+ INTERNAL(450, "InternalError"),
+ MERGE_RESPONSE(500, "MergeResponseError"),
+ QUERY_CANCELLATION(503, "QueryCancellationError"),
+ QUERY_VALIDATION(700, "QueryValidationError"),
+ UNKNOWN_COLUMN(710, "UnknownColumnError"),
+ QUERY_PLANNING(720, "QueryPlanningError"),
+ UNKNOWN(1000, "UnknownError");
+ private static final Logger LOGGER =
LoggerFactory.getLogger(QueryErrorCode.class);
+
+ private static final QueryErrorCode[] BY_ID;
+
+ static {
+ int maxId = -1;
+ for (QueryErrorCode queryErrorCode : QueryErrorCode.values()) {
+ maxId = Math.max(maxId, queryErrorCode.getId());
+ }
+ BY_ID = new QueryErrorCode[maxId + 1];
Review Comment:
Isn't this a little wasteful considering that the error codes form a fairly
sparse set?
##########
pinot-common/src/main/java/org/apache/pinot/common/datablock/DataBlockUtils.java:
##########
@@ -97,18 +98,22 @@ private DataBlockUtils() {
public static MetadataBlock getErrorDataBlock(Exception e) {
if (e instanceof ProcessingException) {
- return getErrorDataBlock(Collections.singletonMap(((ProcessingException)
e).getErrorCode(), extractErrorMsg(e)));
+ int errorCodeId = ((ProcessingException) e).getErrorCode();
+ return getErrorDataBlock(Collections.singletonMap(errorCodeId,
extractErrorMsg(e)));
+ } else if (e instanceof QueryException) {
+ int errorCodeId = ((QueryException) e).getErrorCode().getId();
+ return getErrorDataBlock(Collections.singletonMap(errorCodeId,
extractErrorMsg(e)));
} else {
// TODO: Pass in meaningful error code.
- return
getErrorDataBlock(Collections.singletonMap(QueryException.UNKNOWN_ERROR_CODE,
extractErrorMsg(e)));
+ return getErrorDataBlock(Map.of(QueryErrorCode.UNKNOWN.getId(),
extractErrorMsg(e)));
}
}
private static String extractErrorMsg(Throwable t) {
- while (t.getCause() != null && t.getMessage() == null) {
- t = t.getCause();
+ if (t.getMessage() != null) {
+ return t.getMessage();
}
- return t.getMessage() + "\n" + QueryException.getTruncatedStackTrace(t);
+ return "empty error message";
Review Comment:
Not really sure this string would help in any case? Why not just leave it
blank in this unexpected case?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/reduce/BrokerReduceService.java:
##########
@@ -121,12 +121,12 @@ public BrokerResponseNative
reduceOnDataTable(BrokerRequest brokerRequest, Broke
// Report the servers with conflicting data schema.
if (!serversWithConflictingDataSchema.isEmpty()) {
- String errorMessage = QueryException.MERGE_RESPONSE_ERROR.getMessage() +
": responses for table: " + tableName
+ String errorMessage = "MergeResponseError: responses for table: " +
tableName
Review Comment:
```suggestion
String errorMessage =
QueryErrorCode.MERGE_RESPONSE.getDefaultMessage() + ": responses for table: " +
tableName
```
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseCombineOperator.java:
##########
@@ -190,16 +191,21 @@ protected static RuntimeException
wrapOperatorException(Operator operator, Runti
}
// Otherwise, try to get the segment name to help locate the segment when
debugging query errors.
// Not all operators have associated segment, so do this at best effort.
- IndexSegment segment = operator.getIndexSegment();
- String errorMessage = null;
- if (segment != null) {
- errorMessage = "Caught exception while doing operator: " +
operator.getClass()
- + " on segment: " + segment.getSegmentName();
+ // TODO: Do not use class name here but the operator name explain plan. To
do so, that method must be moved from
+ // multi and single stage operators to the base operator
+ String errorMessage = operator.getIndexSegment() != null
+ ? "Caught exception while doing operator: " + operator.getClass()
+ + " on segment " + operator.getIndexSegment().getSegmentName()
+ : "Caught exception while doing operator: " + operator.getClass();
+
+ QueryErrorCode errorCode;
+ if (e instanceof QueryException) {
+ QueryException queryException = (QueryException) e;
+ errorCode = queryException.getErrorCode();
+ } else {
+ errorCode = QueryErrorCode.QUERY_EXECUTION;
}
-
- if (e instanceof IllegalArgumentException || e instanceof
BadQueryRequestException) {
- throw new BadQueryRequestException(errorMessage, e);
- }
- throw new RuntimeException(errorMessage, e);
+ // TODO: Only include exception message if it is a QueryException.
Otherwise, it might expose sensitive information
+ throw errorCode.asException(errorMessage + ": " + e.getMessage(), e);
Review Comment:
This method is a little strange, why is it either returning a
`RuntimeException` or throwing one?
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseSingleBlockCombineOperator.java:
##########
@@ -112,11 +113,17 @@ protected void processSegments() {
@Override
protected void onProcessSegmentsException(Throwable t) {
_processingException.compareAndSet(null, t);
- if (t instanceof BadQueryRequestException) {
- _blockingQueue.offer(new
ExceptionResultsBlock(QueryException.QUERY_VALIDATION_ERROR, t));
+ ExceptionResultsBlock errBlock;
+ if (t instanceof QueryException) {
+ QueryException queryException = (QueryException) t;
+ QueryErrorMessage errMsg =
QueryErrorMessage.safeMsg(queryException.getErrorCode(),
queryException.getMessage());
+ errBlock = new ExceptionResultsBlock(errMsg);
Review Comment:
Why no log in this branch?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -544,6 +548,19 @@ public static QueryResult runReducer(long requestId,
System.currentTimeMillis() - startTimeMs);
}
+ // TODO: Improve the way the errors are compared
+ private static int compareErrors(Map.Entry<Integer, String> entry1,
Map.Entry<Integer, String> entry2) {
+ int errorCode1 = entry1.getKey();
+ int errorCode2 = entry2.getKey();
+ if (errorCode1 == QueryErrorCode.QUERY_VALIDATION.getId()) {
+ return errorCode1;
+ }
+ if (errorCode2 == QueryErrorCode.QUERY_VALIDATION.getId()) {
+ return errorCode2;
+ }
+ return Integer.compare(errorCode1, errorCode2);
+ }
Review Comment:
Okay nvm I see that the older code was also essentially doing the same thing
but it'd still be good to know if you're aware of the reason behind this.
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/combine/BaseSingleBlockCombineOperator.java:
##########
@@ -65,7 +66,7 @@ protected BaseResultsBlock getNextBlock() {
throw new EarlyTerminationException("Interrupted while merging results
blocks", e);
} catch (Exception e) {
LOGGER.error("Caught exception while merging results blocks (query:
{})", _queryContext, e);
- mergedBlock = new
ExceptionResultsBlock(QueryException.getException(QueryException.INTERNAL_ERROR,
e));
+ mergedBlock = new
ExceptionResultsBlock(QueryErrorMessage.safeMsg(QueryErrorCode.INTERNAL,
e.getMessage()));
Review Comment:
Should the message contain the context that the error occurred while merging
result blocks?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -358,7 +354,7 @@ private String sendRequestToBroker(String query, String
instanceId, String trace
InstanceConfig instanceConfig =
_pinotHelixResourceManager.getHelixInstanceConfig(instanceId);
if (instanceConfig == null) {
LOGGER.error("Instance {} not found", instanceId);
- return QueryException.INTERNAL_ERROR.toString();
+ throw QueryErrorCode.INTERNAL.asException("InternalError");
Review Comment:
```suggestion
throw QueryErrorCode.INTERNAL.asException();
```
##########
pinot-spi/src/main/java/org/apache/pinot/spi/exception/QueryErrorMessage.java:
##########
@@ -0,0 +1,94 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pinot.spi.exception;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.databind.node.ObjectNode;
+import javax.annotation.Nullable;
+import org.apache.pinot.spi.utils.JsonUtils;
+
+
+public class QueryErrorMessage {
+ /**
+ * Error code for the exception, usually obtained from {@link
QueryException}.
+ */
+ private final QueryErrorCode _errCode;
+ /**
+ * User facing error message.
+ *
+ * This message is intended to be shown to the user and should not contain
implementation details (including stack
+ * traces or class names).
+ */
+ private final String _usrMsg;
+ /**
+ * Log message for the exception.
+ *
+ * This message is intended to be logged and may contain implementation
details (including stack traces or class
+ * names) but it must never contain any sensitive information (including
user data).
+ */
+ private final String _logMsg;
+
+ @JsonCreator
+ public QueryErrorMessage(
+ @JsonProperty("e") QueryErrorCode errCode,
+ @JsonProperty("u") @Nullable String usrMsg,
+ @JsonProperty("l") @Nullable String logMsg) {
Review Comment:
Why these single letter JSON property names? Where is this
`QueryErrorMessage` expected to be serde into JSON from?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/executor/ServerQueryExecutorV1Impl.java:
##########
@@ -353,12 +348,15 @@ private InstanceResponseBlock
executeInternal(ServerQueryRequest queryRequest, E
// NOTE most likely the onFailure() callback registered on query
future in InstanceRequestHandler would
// return the error table to broker sooner than here. But in case of
race condition, we construct the error
// table here too.
-
instanceResponse.addException(QueryException.getException(QueryException.QUERY_CANCELLATION_ERROR,
- "Query cancelled on: " + _instanceDataManager.getInstanceId() + "
" + e));
+ instanceResponse.addException(QueryErrorCode.QUERY_CANCELLATION,
+ "Query cancelled on: " + _instanceDataManager.getInstanceId() + "
" + e);
+ } else if (e instanceof QueryException) {
+ LOGGER.info("Caught QueryException while processing requestId: {},
{}", requestId, e.getMessage());
+ instanceResponse.addException(QueryErrorCode.QUERY_VALIDATION,
e.getMessage());
Review Comment:
Why is this a `QUERY_VALIDATION` error?
##########
pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotQueryResource.java:
##########
@@ -277,20 +275,20 @@ private String getQueryResponse(String query, @Nullable
SqlNode sqlNode, String
// Check if the query is a v2 supported query
if (ParserUtils.canCompileWithMultiStageEngine(query, database,
_pinotHelixResourceManager.getTableCache())) {
- return QueryException.getException(QueryException.SQL_PARSING_ERROR,
new Exception(
+ throw QueryErrorCode.SQL_PARSING.asException(
"It seems that the query is only supported by the multi-stage
query engine, please retry the query using "
+ "the multi-stage query engine "
- +
"(https://docs.pinot.apache.org/developers/advanced/v2-multi-stage-query-engine)")).toString();
+ +
"(https://docs.pinot.apache.org/developers/advanced/v2-multi-stage-query-engine)");
} else {
- return QueryException.getException(QueryException.SQL_PARSING_ERROR,
e).toString();
+ throw QueryErrorCode.SQL_PARSING.asException(e);
}
}
String rawTableName = TableNameBuilder.extractRawTableName(tableName);
// Validate data access
AccessControl accessControl = _accessControlFactory.create();
if (!accessControl.hasAccess(rawTableName, AccessType.READ, httpHeaders,
Actions.Table.QUERY)) {
- return QueryException.ACCESS_DENIED_ERROR.toString();
+ throw QueryErrorCode.ACCESS_DENIED.asException("AccessDenied");
Review Comment:
```suggestion
throw QueryErrorCode.ACCESS_DENIED.asException();
```
This is already the default message for the error code right?
##########
pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/QueryScheduler.java:
##########
@@ -247,10 +247,18 @@ private byte[] serializeResponse(ServerQueryRequest
queryRequest, InstanceRespon
* query can not be executed.
*/
protected ListenableFuture<byte[]> immediateErrorResponse(ServerQueryRequest
queryRequest,
- ProcessingException error) {
+ QueryErrorCode errorCode, String message) {
InstanceResponseBlock instanceResponse = new InstanceResponseBlock();
instanceResponse.addMetadata(MetadataKey.REQUEST_ID.getName(),
Long.toString(queryRequest.getRequestId()));
- instanceResponse.addException(error);
+ instanceResponse.addException(errorCode, message);
return Futures.immediateFuture(serializeResponse(queryRequest,
instanceResponse));
}
+
+ protected ListenableFuture<byte[]> shuttingDown(ServerQueryRequest
queryRequest) {
+ return immediateErrorResponse(queryRequest,
QueryErrorCode.SERVER_SHUTTING_DOWN, "ServerShuttingDown");
+ }
+
+ protected ListenableFuture<byte[]> outOfCapacity(ServerQueryRequest
queryRequest) {
+ return immediateErrorResponse(queryRequest,
QueryErrorCode.SERVER_OUT_OF_CAPACITY, "ServerOutOfCapacity");
Review Comment:
nit: can reuse the default message from the enums
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/MultiStageBrokerRequestHandler.java:
##########
@@ -222,16 +222,16 @@ protected BrokerResponse handleRequest(long requestId,
String query, SqlNodeAndO
() -> finalQueryEnvironment.planQuery(query,
sqlNodeAndOptions, requestId),
queryTimer.getRemainingTime());
} catch (TimeoutException | InterruptedException e) {
-
requestContext.setErrorCode(QueryException.BROKER_TIMEOUT_ERROR_CODE);
- return new
BrokerResponseNative(QueryException.BROKER_TIMEOUT_ERROR);
+ requestContext.setErrorCode(QueryErrorCode.BROKER_TIMEOUT);
+ return new BrokerResponseNative(QueryErrorCode.BROKER_TIMEOUT,
"BrokerTimeoutError");
Review Comment:
Nvm, just noticed that this is just updating the code as is, let's limit the
scope of this PR. Although could we use the default error message from the
`QueryErrorCode.BROKER_TIMEOUT` enum instead of hardcoding it here as well?
--
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
