xiangfu0 commented on code in PR #19876:
URL: https://github.com/apache/pinot/pull/19876#discussion_r4289838985


##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/MailboxService.java:
##########
@@ -61,18 +64,16 @@ public class MailboxService {
   ///
   /// We use a cache to ensure the receiving mailbox are not leaked in the 
cases where the corresponding OpChain is
   /// either never registered or died before the sender finished sending data.
+  ///
+  /// A mailbox that no operator ever read is an orphan: either its blocks 
were meant for another instance and reached
+  /// this one through a stale connection of the sender, or they arrived after 
the query had ended here. Orphans are
+  /// counted when they are evicted, at least 
[#DANGLING_RECEIVING_MAILBOX_EXPIRY_SECONDS] after the last stream for
+  /// them opened (the cache drops expired entries only when it is used again).
   private final Cache<String, ReceivingMailbox> _receivingMailboxCache =

Review Comment:
   Worth keeping in mind that this counter only moves when the cache is touched 
after an entry has expired. On an idle instance the orphan count can lag by 
several minutes, or not fire at all until the next query. The doc comment 
already calls this out, so just flagging it as expected behavior, not a bug.



##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/BaseMailboxReceiveOperator.java:
##########
@@ -77,7 +80,9 @@ public BaseMailboxReceiveOperator(OpChainExecutionContext 
context, MailboxReceiv
         asyncStreams.add(asyncStream);
         _receivingStats.add(asyncStream._mailbox.getStatMap());
       }
-      _multiConsumer = new BlockingMultiStreamConsumer.OfMseBlock(context, 
asyncStreams, senderStageId);
+      List<String> mailboxIds = _mailboxIds;

Review Comment:
   Small thing: this local copy of `_mailboxIds` isn't needed. A lambda can 
capture the `_mailboxIds` field directly, so you could pass `() -> 
describeSenders(mailboxInfos.getMailboxInfos(), _mailboxIds)` and drop the 
extra variable.



-- 
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]

Reply via email to