MartijnVisser commented on PR #28315: URL: https://github.com/apache/flink/pull/28315#issuecomment-4631530809
Addressed the review feedback that the regression test didn't actually exercise the added behavior. The previous test's behavioral assertion could be satisfied by the pre-existing `ClientHandler.channelInactive` path completing the future on close, so it could pass even without the fix. New commit (to be squashed on merge): - Adds `testCloseFailsInFlightRequestFutureViaPendingRequestFutures`, which constructs the `RestClient` with a **deferring executor** that captures the response-composition stages without running them. The terminal response future is therefore never wired to the handler's `jsonFuture`, so `channelInactive` cannot complete it — the only remaining completion path is the close-side `pendingRequestFutures` handling. It asserts the failure *identity* (`ExecutionException` → `IllegalStateException` with `CLOSED_BEFORE_REQUEST_COMPLETED_MESSAGE`), not merely that the future failed. - Keeps the original real-executor test as the end-to-end companion (it exercises the normal path including `channelInactive`). Red/green verified locally: with the close-side `pendingRequestFutures` handling removed, the new test fails deterministically with `TimeoutException` after ~10s (bounded `failsWithin`, never hangs); with the fix it passes in ~0.2s. Full `RestClientTest` is 11/11; `spotless:check` and `checkstyle:check` pass. Note: the new test couples to the contract that the client `executor` is used only for the post-connect composition stages, so future `submitRequest` refactors should re-check it. -- 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]
