yashmayya commented on code in PR #16919:
URL: https://github.com/apache/pinot/pull/16919#discussion_r2388847085
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentObserver.java:
##########
@@ -84,6 +84,9 @@ public void onNext(MailboxContent mailboxContent) {
cancelStream();
break;
case FIRST_ERROR:
+
_responseObserver.onNext(MailboxStatus.newBuilder().setMailboxId(mailboxId)
Review Comment:
Why do we need to send back an early termination signal on receiving an
error block? The error block would be coming from the sender side itself and so
it should be able to do whatever cleanup is required, right?
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/channel/MailboxContentObserver.java:
##########
@@ -84,6 +84,9 @@ public void onNext(MailboxContent mailboxContent) {
cancelStream();
break;
case FIRST_ERROR:
+
_responseObserver.onNext(MailboxStatus.newBuilder().setMailboxId(mailboxId)
+
.putMetadata(ChannelUtils.MAILBOX_METADATA_REQUEST_EARLY_TERMINATE,
"true").build());
+ cancelStream();
Review Comment:
Just for my understanding, we're calling `onCompleted` inside the
`cancelStream` method to close the gRPC stream (and not channel or connection
🙂)? So then would we never see the `ERROR` case if `FIRST_ERROR` is itself
closing the stream?
--
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]
