xiangfu0 commented on code in PR #19876:
URL: https://github.com/apache/pinot/pull/19876#discussion_r4278291022
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/mailbox/MailboxServiceTest.java:
##########
@@ -85,6 +90,47 @@ public void setUpStats() {
_stats = new StatMap<>(MailboxSendOperator.StatKey.class);
}
+ /// A receiving mailbox that no operator ever read is counted as an orphan
when the cache drops it, which is how data
+ /// that reached the wrong instance, or arrived after its query ended,
shows. A broker counts in the broker metrics.
+ @Test
+ public void testOrphanedReceivingMailboxesAreCounted() {
+ ServerMetrics.deregister();
+ ServerMetrics.register(new
ServerMetrics(PinotMetricUtils.getPinotMetricsRegistry()));
+ BrokerMetrics.register(new
BrokerMetrics(PinotMetricUtils.getPinotMetricsRegistry()));
Review Comment:
Minor test hygiene: this registers a fresh `BrokerMetrics` but never
deregisters it, and it doesn't restore the previous
`ServerMetrics`/`BrokerMetrics` instances. Since these are JVM-wide singletons,
a later test in the same run could see the swapped registry. Consider restoring
both in the `finally` (or deregistering the broker one too) to keep the test
self-contained.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/runtime/operator/utils/BlockingMultiStreamConsumer.java:
##########
@@ -369,6 +388,32 @@ protected ReceivingMailbox.MseBlockWithStats onTimeout() {
return onException(QueryErrorCode.EXECUTION_TIMEOUT, errMsg);
}
+ /// Names the senders whose end of stream has not arrived, so that a
timeout caused by one lost stream (for example
+ /// one sent to the wrong server) points to the sender and its address
rather than to the whole child stage.
+ private String describeMissingSenders() {
+ int numMissing = _mailboxes.size();
+ if (numMissing == 0) {
+ return "";
+ }
+ StringBuilder message = new StringBuilder(" (no end of stream from
").append(numMissing).append(" of ")
+ .append(_numSenders).append(" senders");
+ Map<Object, String> senderDescriptions = _senderDescriptions.get();
+ int numNamed = 0;
+ for (AsyncStream<ReceivingMailbox.MseBlockWithStats> mailbox :
_mailboxes) {
+ String description = senderDescriptions.get(mailbox.getId());
+ if (description == null) {
+ continue;
Review Comment:
Minor: senders with a null description are skipped via `continue` but still
counted in `numMissing`. So a message could read "no end of stream from 10 of
46 senders" yet name fewer than 10 with no trailing "...", which reads a bit
oddly. In practice descriptions should always be present, so this is cosmetic —
worth a short comment noting the assumption, or counting only the named ones.
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/MailboxService.java:
##########
@@ -295,6 +296,34 @@ public void releaseReceivingMailbox(ReceivingMailbox
mailbox) {
_receivingMailboxCache.invalidate(mailbox.getId());
}
+ @VisibleForTesting
+ void onReceivingMailboxEvicted(String mailboxId, ReceivingMailbox
receivingMailbox) {
+ int numPendingBlocks = receivingMailbox.getNumPendingBlocks();
+ boolean neverRead = !receivingMailbox.hasReader();
+ if (neverRead) {
+ recordOrphanedReceivingMailbox();
Review Comment:
Minor/discussion: this counts any never-read mailbox on eviction, including
ones with zero pending blocks (e.g. a receiver registered the mailbox but the
sender never connected, or the query was cancelled). Those aren't really
"misrouted data" cases, so the meter may pick up some normal-cancellation
noise. If you only care about lost data, gating on `numPendingBlocks > 0` might
make the signal cleaner — your call depending on what you want the meter to
mean.
--
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]
