gortiz commented on code in PR #12704:
URL: https://github.com/apache/pinot/pull/12704#discussion_r1576213860
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/mailbox/ReceivingMailbox.java:
##########
@@ -72,11 +81,53 @@ public String getId() {
return _id;
}
+ /**
+ * Offers a raw block into the mailbox within the timeout specified, returns
whether the block is successfully added.
+ * If the block is not added, an error block is added to the mailbox.
+ * <p>
+ * Contrary to {@link #offer(TransferableBlock, long)}, the block may be an
+ * {@link TransferableBlock#isErrorBlock() error block}.
+ */
+ public ReceivingMailboxStatus offerRaw(ByteBuffer byteBuffer, long timeoutMs)
+ throws IOException {
+ TransferableBlock block;
+ long now = System.currentTimeMillis();
+ _stats.merge(StatKey.WAIT_CPU_TIME_MS, now - _lastArriveTime);
+ _lastArriveTime = now;
+ _stats.merge(StatKey.DESERIALIZED_BYTES, byteBuffer.remaining());
+ _stats.merge(StatKey.DESERIALIZED_MESSAGES, 1);
+
+ now = System.currentTimeMillis();
+ DataBlock dataBlock = DataBlockUtils.getDataBlock(byteBuffer);
+ _stats.merge(StatKey.DESERIALIZATION_TIME_MS, System.currentTimeMillis() -
now);
+
+ if (dataBlock instanceof MetadataBlock) {
+ Map<Integer, String> exceptions = dataBlock.getExceptions();
+ if (exceptions.isEmpty()) {
+ block = TransferableBlockUtils.wrap(dataBlock);
+ } else {
+
setErrorBlock(TransferableBlockUtils.getErrorTransferableBlock(exceptions));
+ return ReceivingMailboxStatus.FIRST_ERROR;
Review Comment:
Older code was treating this case in a bit different way. This method was
mostly moved from MailboxContentObserver, which in this case the coded was
doing:
```
_mailbox.setErrorBlock(TransferableBlockUtils.getErrorTransferableBlock(exceptions));
return;
```
The only reason to add this `FIRST_ERROR` enum is to keep the same behavior
in `MailboxContentObserver`. I would prefer to just return `ERROR` here, but
that would have the of printing the following log in `MailboxContentObserver`.
```
LOGGER.warn("Mailbox: {} already errored out (received error block
before)", mailboxId);
```
I guess we can remove this new enum and change the code in
`MailboxContentObserver` a little bit.
> Also seems this message doesn't apply to in-memory mailbox, some comments
would help explain it
`InMemorySendingMailbox` does never call this code because this enum is only
returned when there is an error deserializing the message. Given messages are
always heap based in InMemorySendingMailbox, it can never fail in that way.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
