gortiz commented on code in PR #15037:
URL: https://github.com/apache/pinot/pull/15037#discussion_r1956111050
##########
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:
Yes. That is the key idea of this refactor. In both MSE and SSE errors (and
with error I mean the abstract concept, not java.lang.error) are detected and
transmitted downstream (to the parent operators). But there are clearly two
different states/phases/worlds/whatever where the error may be:
1. As a `java.lang.Throwable`, which is eventually caught and transform into
2. An error message inside an error block. Depending on the block
implementation, this abstract error can be stored in very different Java
objects (including ProcessingExceptions, but also Map.Entry whose key is an
error code and value is the String containing the error message).
Not all errors communicated to the user require starting as an actual
`java.lang.Throwables` being thrown. Sometimes the error situation is detected
and an error code is created. In master this sometimes means we need to create
an Exception instance that is never being thrown, which is a bit strange.
Even more strange is that sometimes we allocate an static exception and just
send that exception as error message. That exception includes a completely fake
stack trace (whose root is the static section of the class that contains the
static Exception reference, which is super misleading).
In the code in master we:
1. A logical error can be logged 0, 1 or multiple times on the node (server,
broker, etc) that throws the exception.
2. When the exception is wrapped into a block, we include a partial stack
trace.
3. When error blocks are found, sometimes we log their error messages. This
means we end up logging the traces an extra time. Even worse, given the blocks
are usually sent to other nodes (in SSE to the broker and in MSE to both broker
and other servers) we end up logging the stacktrace of another process, which
is super misleading.
Instead what we should do is:
1. Only allocate exceptions when they are immediately going to be thrown
2. Most of the times, these exceptions will be caught and converted into an
error message. At that point, we have the exact stack trace, and there is where
we should log it (if needed). If we want to log it, we know what we want to
log, so we can calculate the `logMsg` the QueryErrorMessage should contain.
4. Once converted to a QueryErrorMessage, we don't need the stack trace as
it should already have been logged. What we need is the message we want to
present to the user (which should never include the stack trace) and the
message we want to log in other modules (including other servers and brokers)
when the block is received. This log message doesn't require the stack trace
because it is not going to be useful in other nodes.
--
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]
