yashmayya opened a new pull request, #20096: URL: https://github.com/apache/pinot/pull/20096
## Problem Fixes #20094. `SingleConnectionBrokerRequestHandlerFailureDetectorTest.testServerWhoseQueryEngineIsStuckStaysInRouting` fails on every run on master. It is not flaky. #19943 changed `InstanceRequestHandler` to call `QueryScheduler.submitResponse()`. The shared fixture `QueryServerTestUtils.newQueryServer()` (from #19452) still stubbed the deprecated `submit()` on a Mockito mock. The mock returns `null` from `submitResponse()`. The handler then throws an NPE and answers each query at once with an error response. So the "stuck" server is not stuck, and the query does not time out. Three gaps made the break easy to miss: 1. There were two copies of the scheduler wiring: the shared fixture and a private copy in `QueryRoutingTest`. #19943 updated only the private copy. 2. The fixture took an untyped `Answer<?>`. Callers still returned `byte[]` futures, and the compiler did not catch it. 3. `QueryRoutingTest.testPingIsAnsweredPromptlyWhileTheQueryEngineIsStuck` gets the same NPE, but it passes. It checks `getStatus() == IN_PROGRESS`. The status stays `IN_PROGRESS` until a caller waits for the query, even after the server answers. So pinot-core CI (Unit Test Set 1) stayed green. The NPE also made the healthy server in `testEveryFrozenServerIsTakenOutOfRouting` answer with an error. That test passed anyway. ## Fix The change is in test code only. - `QueryServerTestUtils.newQueryServer()` takes a typed `Function<ServerQueryRequest, ListenableFuture<DataBuffer>>`. A `byte[]` future no longer compiles. - The fixture uses a real `QueryScheduler` subclass that overrides `submitResponse()`, not a mock that stubs it. `QueryScheduler` sends both submit methods to this override, so either method returns the response that the test supplies. A mock returns `null` from any method that is not stubbed, and that is how this fixture broke. - `QueryRoutingTest` builds its servers with `QueryServerTestUtils`. Its private scheduler mock and its copy of `startAndGetPort()` are removed, so only one copy of the wiring is left. - The stuck-engine test in `QueryRoutingTest` also checks that the stuck query has no response. - The broker test fixtures return `DataBuffer` futures. ## Testing - Before the fix: the broker test fails as in #20094. `QueryRoutingTest` passes 20/20 with the NPE in its log. - After the fix: all 32 test classes in `org.apache.pinot.core.transport`, `org.apache.pinot.core.query.scheduler` and `org.apache.pinot.broker.requesthandler` pass. The logs have no NPE. - With the old mock put back in the fixture, the broker test fails, and 13 of 20 `QueryRoutingTest` tests fail. The stuck-engine test fails on the new check: `The stuck query must still be unanswered expected [0] but found [1]`. -- 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]
