Vamsi-klu commented on PR #18146: URL: https://github.com/apache/pinot/pull/18146#issuecomment-4912317558
Traced getNextBlockSingleThread() against mergeResults()/processSegments() and it lines up: acquire/release in the finally, isQuerySatisfied early break, per-segment error propagation, and the currentTimeMillis() >= endTimeMs guard is equivalent to the original waitTimeMs <= 0 check. The Phaser segment-lifetime concern doesn't apply here since everything runs synchronously before getNextBlock() returns, and leaving the worker CPU/mem atomics at 0 to avoid double-counting the calling thread (already measured by InstanceResponseOperator) is the right call. One real parity gap worth tightening: the timeout return, attachExecutionStats(getTimeoutResultsBlock(i)), and the segment-error return, attachExecutionStats(resultsBlock), both call attachExecutionStats() directly, whereas the multi-thread path routes mergeResults()'s result through checkTerminateExceptionAndAttachExecutionStats(). So a query that's explicitly terminated but also times out or hits a segment error would report EXECUTION_TIMEOUT / the segment error instead of the TerminationException reason. Routing those two returns through checkTerminateExceptionAndAttachExecutionStats() would keep behavior identical to the existing path. Tests: the timeout return and the operator-throws path are exactly the branches Codecov flags uncovered, and since timeout handling is the part the earlier review pushed back on, a couple of targeted _numTasks==1 cases (endTimeMs in the past -> timeout block; a throwing segment operator -> wrapped error block) would lock it in. CombineSlowOperatorsTest / CombineErrorOperatorsTest already have the harness. -- 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]
