vrajat commented on code in PR #15037: URL: https://github.com/apache/pinot/pull/15037#discussion_r1954521714
########## 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"), Review Comment: Style question: Is this sorted on any criteria ? If not, can the list be sorted either by error code or name ? Error code is better. ########## pinot-spi/src/main/java/org/apache/pinot/spi/exception/EarlyTerminationException.java: ########## @@ -22,7 +22,7 @@ * 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 PinotRuntimeException { Review Comment: Should this be derived from `QueryException` ? I checked usages and it is used in query processing only. ########## pinot-query-runtime/src/main/java/org/apache/pinot/query/service/server/QueryServer.java: ########## @@ -174,9 +175,10 @@ public void explain(Worker.QueryRequest request, StreamObserver<Worker.ExplainRe requestMetadata = QueryPlanSerDeUtils.fromProtoProperties(request.getMetadata()); } catch (Exception e) { LOGGER.error("Caught exception while deserializing request metadata", e); + String errorMsg = "Caught exception while deserializing request metadata: " + e.getMessage(); responseObserver.onNext(Worker.ExplainResponse.newBuilder() - .putMetadata(CommonConstants.Explain.Response.ServerResponseStatus.STATUS_ERROR, - QueryException.getTruncatedStackTrace(e)).build()); + .putMetadata(CommonConstants.Explain.Response.ServerResponseStatus.STATUS_ERROR, errorMsg) Review Comment: Making sure I understand the pattern here. `LOGGER.error` prints the stack trace. However the stack trace is not added to the user message. ########## pinot-common/src/main/java/org/apache/pinot/common/response/broker/BrokerQueryErrorMessage.java: ########## @@ -18,24 +18,39 @@ */ package org.apache.pinot.common.response.broker; +import com.fasterxml.jackson.annotation.JsonCreator; import com.fasterxml.jackson.annotation.JsonProperty; +import org.apache.pinot.spi.exception.QueryErrorCode; +import org.apache.pinot.spi.exception.QueryErrorMessage; /** * This class represents an exception using a message and an error code. + * + * This is only used to serialize the error message and error code when a broker sends an error message to the client. + * In other cases use {@link QueryErrorMessage} instead. */ -public class QueryProcessingException { +public class BrokerQueryErrorMessage { Review Comment: Can you explain more why both `BrokerQueryErrorMessage` and `QueryErrorMessage` is required ? Is the main difference that `QueryErrorMessage has a user/log message ? Also in `QueryErrorMessage` are log & user message different often ? -- 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
