gortiz commented on code in PR #19682:
URL: https://github.com/apache/pinot/pull/19682#discussion_r4119489682
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcSendingMailbox.java:
##########
@@ -297,7 +297,8 @@ public void cancel(Throwable t) {
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
MseBlock errorBlock = ErrorMseBlock.fromError(
- QueryErrorCode.QUERY_CANCELLATION, "Cancelled by sender with
exception: " + msg);
+ QueryErrorCode.fromThrowable(t, QueryErrorCode.QUERY_CANCELLATION),
Review Comment:
Core fix. The only caller of cancel(t) is OpChainSchedulerService.onFailure.
When the root returns an error block, runJob throws
errorBlock.getMainErrorCode().asException(...), so t is a QueryException that
carries 245. Before this change the code was dropped here and replaced with 503.
Race: MailboxSendOperator tries to send the real error EOS. If a
TerminationException (the CPU kill) unwinds that send, _senderSideClosed stays
false and this cancel sends its own EOS. Now the EOS carries the same code, so
the race no longer changes the broker result. Only the code is fixed, not the
race; that is enough for #19681.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcSendingMailbox.java:
##########
@@ -297,7 +297,8 @@ public void cancel(Throwable t) {
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
MseBlock errorBlock = ErrorMseBlock.fromError(
- QueryErrorCode.QUERY_CANCELLATION, "Cancelled by sender with
exception: " + msg);
+ QueryErrorCode.fromThrowable(t, QueryErrorCode.QUERY_CANCELLATION),
+ "Cancelled by sender with exception: " + msg);
Review Comment:
Minor: the message still says 'Cancelled by sender with exception:', but the
code can now be SERVER_RESOURCE_LIMIT_EXCEEDED, EXECUTION_TIMEOUT, INTERNAL and
so on. The message also wraps the full error-block text ('Error block from
stage N ... Msg: {CODE=...}. Stats: ...'), so users can see a long, nested
message. This is not a blocker. Maybe forward the message without the
'Cancelled' prefix when the code is not QUERY_CANCELLATION.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/GrpcSendingMailbox.java:
##########
@@ -134,8 +134,8 @@ public void send(MseBlock.Eos block, List<DataBuffer>
serializedStats) {
// and must always reach the receiver, so they bypass the back-pressure
gate. Bypassing also disables the
// cooperative termination poll inside [#awaitReady]; without it, a
terminate signal raised while the sender is
// mid-way through pushing an error EOS would unwind [#sendInternal] with
a TerminationException, leave
- // [#_senderSideClosed] false, and let [#cancel] run and overwrite the
original error code with
- // QUERY_CANCELLATION on the receiver side.
+ // [#_senderSideClosed] false, and let [#cancel] run and replace the
original error code with
+ // QUERY_CANCELLATION if its throwable does not carry a QueryErrorCode.
Review Comment:
Behaviour change that the PR does not describe: every QueryException code
now reaches the receiver, not only resource-limit codes. EXECUTION_TIMEOUT from
a TerminationException, INTERNAL from an operator bug, and similar codes no
longer become 503. I think this is correct, but it changes the error codes and
metrics that users see. Please say so in the PR description.
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/mailbox/GrpcSendingMailboxTest.java:
##########
@@ -50,19 +51,55 @@
import org.apache.pinot.segment.spi.memory.DataBuffer;
import org.apache.pinot.segment.spi.memory.PinotByteBuffer;
import org.apache.pinot.spi.exception.QueryErrorCode;
+import org.apache.pinot.spi.exception.QueryException;
import org.apache.pinot.spi.exception.TerminationException;
import org.apache.pinot.spi.query.QueryThreadContext;
+import org.mockito.ArgumentCaptor;
import org.mockito.Mockito;
import org.testng.Assert;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertTrue;
public class GrpcSendingMailboxTest {
+ @Test(dataProvider = "cancellationErrors")
+ @SuppressWarnings("unchecked")
+ public void cancelPreservesQueryErrorCode(Exception exception,
QueryErrorCode expectedCode)
+ throws IOException {
Review Comment:
The tests check the mapping (QueryException -> its code, anything else ->
503) on both mailboxes. They do not reproduce the race from #19681. That is
acceptable for a mapping fix. Consider adding a data-provider row for a
TerminationException (for example EXECUTION_TIMEOUT), which is the other real
input to cancel(), and a row for null t in the gRPC path.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/InMemorySendingMailbox.java:
##########
@@ -120,8 +120,8 @@ public void cancel(Throwable t) {
_receivingMailbox = _mailboxService.getReceivingMailbox(_id);
}
_receivingMailbox.setErrorBlock(
- ErrorMseBlock.fromException(new QueryCancelledException(
- "Cancelled by sender with exception: " + t.getMessage())),
List.of());
+ ErrorMseBlock.fromError(QueryErrorCode.fromThrowable(t,
QueryErrorCode.QUERY_CANCELLATION),
Review Comment:
Same fix for the in-memory path. I checked the swap from fromException(new
QueryCancelledException(...)) to fromError(code, msg). For a QueryException,
fromException uses the plain message and no trace, so the block contents stay
the same. Only the code changes.
Pre-existing nit: t.getMessage() throws an NPE if t is null, but the gRPC
path guards against it. OpChain.cancel always passes a non-null value today.
--
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]
