albertobastos commented on code in PR #15533:
URL: https://github.com/apache/pinot/pull/15533#discussion_r2042314241
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcSendingMailbox.java:
##########
@@ -131,9 +132,8 @@ public void cancel(Throwable t) {
try {
String msg = t != null ? t.getMessage() : "Unknown";
// NOTE: DO NOT use onError() because it will terminate the stream, and
receiver might not get the callback
- _contentObserver.onNext(toMailboxContent(
- ErrorMseBlock.fromException(new RuntimeException("Cancelled by
sender with exception: " + msg, t)),
- List.of()));
+ _contentObserver.onNext(toMailboxContent(ErrorMseBlock.fromException(
+ new QueryCancelledException("Cancelled by sender with exception: " +
msg)), List.of()));
Review Comment:
Yeah, well seen. Replaced with a `ErrorMseBlock.fromError(code, msg)` method.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/exception/QueryErrorCode.java:
##########
@@ -57,6 +57,8 @@ public enum QueryErrorCode {
UNKNOWN_COLUMN(710, "UnknownColumnError"),
/// Error while planning the query. For example, trying to run a colocated
join on non-colocated tables.
QUERY_PLANNING(720, "QueryPlanningError"),
+ /// Query already errored out.
+ ERRORED_OUT(800, "ErroredOutError"),
Review Comment:
I just added it because felt like where the code was throwing a
`RuntimeException`, but I think we can consider the "trying to keep using a
mailbox already errored out" situation as an `INTERNAL`.
##########
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:
You're right, moved to a private method in `ErrorMseBlock` itself and
including the stack trace only for `UNKNOWN` errors. `INTERNAL` ones should
have a message with enough information to know what and where happened.
##########
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:
Yeah, just replaced with `INTERNAL`.
##########
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:
I agree, using the same strategy as in `GrpcSendingMailbox`.
--
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
