Jackie-Jiang commented on code in PR #19268:
URL: https://github.com/apache/pinot/pull/19268#discussion_r3790763317
##########
pinot-query-runtime/src/test/java/org/apache/pinot/query/mailbox/MailboxServiceTest.java:
##########
@@ -560,6 +563,12 @@ public void testRemoteBufferFull()
for (int i = 0; i < ReceivingMailbox.DEFAULT_MAX_PENDING_BLOCKS; i++) {
sendingMailbox.send(OperatorTestUtil.block(DATA_SCHEMA, new
Object[]{"0"}));
}
+ // Wait until the buffer is full before sending the next block, so that
the remaining deadline is spent parked on
+ // the full buffer instead of racing the block delivery. This also
guarantees the error block comes from the
+ // buffer-full timeout rather than from the deadline cancelling the stream
mid-delivery.
Review Comment:
Good catch, you're right. The wait only proves the first five blocks
arrived; `send` returns once the block is handed to gRPC, and if the deadline
expires before the receiver enters `offerData`,
`MailboxContentObserver.onError` installs an INTERNAL error that satisfies
every assertion below — a silent false pass that never exercises the
buffer-full timeout.
Fixed by asserting the error code, which cleanly separates the two paths
(buffer-full reports `EXECUTION_TIMEOUT`, the deadline cancel reports
`INTERNAL`):
```java
assertEquals(((ErrorMseBlock) block).getErrorMessages().keySet(),
Set.of(QueryErrorCode.EXECUTION_TIMEOUT));
```
I went with the full `keySet()` rather than `containsKey` so a second error
code accumulating on the block also fails. I did not take the "synchronize on
the offer being parked" alternative: the park count is `_pendingData`, which is
private with no accessor, so the assertion is the practical form. The
overstated claim has also been removed from the code comment and the PR
description.
--
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]
