Copilot commented on code in PR #16899:
URL: https://github.com/apache/pinot/pull/16899#discussion_r2379727781
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/exchange/BlockExchange.java:
##########
@@ -174,9 +174,15 @@ protected void sendBlock(SendingMailbox sendingMailbox,
MseBlock.Data block)
protected abstract void route(List<SendingMailbox> destinations,
MseBlock.Data block)
throws IOException, TimeoutException;
- // Called when the OpChain gracefully returns.
- // TODO: This is a no-op right now.
+ @Override
public void close() {
+ for (SendingMailbox sendingMailbox : _sendingMailboxes) {
+ try {
+ sendingMailbox.close();
+ } catch (Exception e) {
+ LOGGER.debug("Exception while cancelling mailbox: {}", sendingMailbox,
e);
Review Comment:
The log message says 'cancelling mailbox' but the code is calling close(),
not cancel(). The message should be updated to 'Exception while closing
mailbox' to accurately reflect the operation being performed.
```suggestion
LOGGER.debug("Exception while closing mailbox: {}", sendingMailbox,
e);
```
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/InMemorySendingMailbox.java:
##########
@@ -135,4 +136,11 @@ public boolean isTerminated() {
public String toString() {
return "m" + _id;
}
+
+ @Override
+ public void close() {
+ if (!isTerminated()) {
+ cancel(new RuntimeException("Closed without being
completed").fillInStackTrace());
Review Comment:
The error message 'Closed without being completed' is unclear and doesn't
provide enough context about why this is problematic or what the expected
behavior should be. Consider a more descriptive message like 'SendingMailbox
closed before completion or cancellation'.
```suggestion
cancel(new RuntimeException("SendingMailbox closed before completion
or cancellation").fillInStackTrace());
```
--
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]
