rohityadav1993 commented on code in PR #19120:
URL: https://github.com/apache/pinot/pull/19120#discussion_r4044526773
##########
pinot-core/src/test/java/org/apache/pinot/core/operator/combine/CombineSlowOperatorsTest.java:
##########
@@ -170,13 +164,85 @@ public void testCancelGroupByOrderByCombineOperator() {
testCancelCombineOperator(combineOperator, ready);
}
+ @Test
+ public void testCancelStreamingSelectionOrderByCombineOperator() {
+ CountDownLatch ready = new CountDownLatch(1);
+ List<Operator> operators = getOperators(ready, minMaxSegmentSupplier());
+ QueryContext queryContext =
QueryContextConverterUtils.getQueryContext("SELECT * FROM testTable ORDER BY
column");
+ queryContext.setEndTimeMs(System.currentTimeMillis() + 10000);
+ // Single-stage mode: nextBlock() drives the whole merge synchronously on
the (cancellable) caller thread.
+ StreamingSelectionOrderByCombineOperator combineOperator =
+ new StreamingSelectionOrderByCombineOperator(operators, queryContext,
_executorService, false);
+ testCancelCombineOperator(combineOperator, ready, operators);
+ }
+
+ @Test
+ public void
testCancelStreamingSelectionOrderByCombineOperatorStreamingMode() {
+ CountDownLatch ready = new CountDownLatch(1);
+ List<Operator> operators = getOperators(ready, minMaxSegmentSupplier());
+ QueryContext queryContext =
QueryContextConverterUtils.getQueryContext("SELECT * FROM testTable ORDER BY
column");
+ queryContext.setEndTimeMs(System.currentTimeMillis() + 10000);
+ // Streaming (MSE-leaf) mode: the first getNextBlock() still drives the
merge on the caller thread, so the same
+ // interrupt-on-cancel path applies and must surface an
ExceptionResultsBlock.
+ StreamingSelectionOrderByCombineOperator combineOperator =
+ new StreamingSelectionOrderByCombineOperator(operators, queryContext,
_executorService, true);
+ testCancelCombineOperator(combineOperator, ready, operators);
+ }
+
+ /// The merge loop drains its heap on the caller thread and, for
single-block cursors, may never re-enter a child
+ /// operator, so it must check the query deadline itself. With an
already-expired deadline the operator must surface a
+ /// timeout <b>before</b> activating any child - asserted via {@code
_operationInProgress}, which also keeps the test
+ /// from passing vacuously if the timeout came from somewhere else.
+ @Test
+ public void testStreamingSelectionOrderByCombineOperatorHonorsDeadline() {
+ List<Operator> operators = getOperators(null, minMaxSegmentSupplier());
Review Comment:
Fixed in f3f2db3 - passes a fresh CountDownLatch(1) that's never awaited
instead of null.
Correction: this can't actually NPE - the null guard predates this PR and
still serves other tests. The real regression mode is worse for CI legibility:
SlowOperator sleeps 3,600,000 ms, so a deadline regression hangs this test for
an hour instead of failing. Not adding a @Test timeout for that since it
changes test semantics and wasn't asked for, but can if you'd like.
--
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]
