gortiz commented on code in PR #15533:
URL: https://github.com/apache/pinot/pull/15533#discussion_r2042154801
##########
pinot-spi/src/main/java/org/apache/pinot/spi/exception/QueryErrorCode.java:
##########
@@ -153,4 +155,14 @@ public boolean isClientError() {
return false;
}
}
+
+ public boolean isIncludeStackTrace() {
+ switch (this) {
+ case INTERNAL:
+ case UNKNOWN:
+ return true;
+ default:
+ return false;
+ }
+ }
Review Comment:
I'm not sure if including the stack trace should be dependent on the error
code, but even if it is, I don't think it is a decision that should be taken
here but instead on the class that actually uses this method. Specifically, the
one that decides whether the stack should be included is ErrorMseBlock, so IMHO
this method should be in that class instead.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentObserver.java:
##########
@@ -114,8 +115,8 @@ public void onError(Throwable t) {
LOGGER.warn("Error on receiver side", t);
if (_mailbox != null) {
_mailbox.setErrorBlock(
- ErrorMseBlock.fromException(new RuntimeException("Cancelled by
sender", t)),
- Collections.emptyList());
+ ErrorMseBlock.fromException(new QueryCancelledException(
Review Comment:
A factory method like ErrorMseBlock.fromErrMsg(ErrorCode, String) would be
useful here as well
##########
pinot-spi/src/main/java/org/apache/pinot/spi/exception/QueryErrorCode.java:
##########
@@ -153,4 +155,14 @@ public boolean isClientError() {
return false;
}
}
+
+ public boolean isIncludeStackTrace() {
+ switch (this) {
+ case INTERNAL:
+ case UNKNOWN:
+ return true;
+ default:
+ return false;
+ }
+ }
Review Comment:
Now, about when we want to include the stack trace in the error block: We
may want to include it if it is an unknown error, but why do we want to do that
when it is internal?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/InMemorySendingMailbox.java:
##########
@@ -87,9 +89,10 @@ private void sendPrivate(MseBlock block, List<DataBuffer>
serializedStats)
case CANCELLED:
throw new QueryCancelledException(String.format("Mailbox: %s already
cancelled from upstream", _id));
case ERROR:
- throw new RuntimeException(String.format("Mailbox: %s already errored
out (received error block before)", _id));
+ throw new QueryException(QueryErrorCode.ERRORED_OUT, String.format(
+ "Mailbox: %s already errored out (received error block before)",
_id));
Review Comment:
I would prefer not to introduce new error codes if unnecessary. Are we sure
we need this one here? Couldn't we just report it as internal?
--
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
