XComp commented on code in PR #23515: URL: https://github.com/apache/flink/pull/23515#discussion_r1360500601
########## flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java: ########## @@ -311,6 +316,8 @@ void testCancelExecutionInteractiveMode() throws Exception { terminal.raise(Terminal.Signal.INT); CommonTestUtils.waitUntilCondition( () -> outputStream.toString().contains(CliStrings.MESSAGE_HELP)); + // Prevent NPE when closing the terminal. See FLINK-33116 for more information. + closedLatch.await(30, TimeUnit.SECONDS); Review Comment: We don't want to use timeouts in the tests anymore. The reason is that if we run into a deadlock, the CI tools would print a thread dump at the end which would reveal more information on where the deadlock happened. Providing a timeout in the test would result in an early failure without any thread dump ([Flink's Coding Guidelines](https://flink.apache.org/how-to-contribute/code-style-and-quality-common/#avoid-timeouts-in-junit-tests) for reference). ########## flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java: ########## @@ -300,6 +303,8 @@ void testCancelExecutionInteractiveMode() throws Exception { try { client.executeInInteractiveMode(); } catch (Exception ignore) { + } finally { Review Comment: I'm wondering whether we should use `CompletableFuture` here, instead. That way, we could collect any `RuntimeException` (which would be otherwise swallowed by the concurrent execution). WDYT? :thinking: ########## flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java: ########## @@ -290,6 +292,7 @@ void testCancelExecutionInteractiveMode() throws Exception { Path historyFilePath = historyTempFile(); InputStream inputStream = new ByteArrayInputStream("SELECT 1;\nHELP;\n ".getBytes()); OutputStream outputStream = new ByteArrayOutputStream(248); + CountDownLatch closedLatch = new CountDownLatch(1); Review Comment: nit: for this, `OneShotLatch` is available. But `CountDownLatch(1)` does the same ¯\_(ツ)_/¯ ########## flink-table/flink-sql-client/src/test/java/org/apache/flink/table/client/cli/CliClientTest.java: ########## @@ -290,6 +292,7 @@ void testCancelExecutionInteractiveMode() throws Exception { Path historyFilePath = historyTempFile(); InputStream inputStream = new ByteArrayInputStream("SELECT 1;\nHELP;\n ".getBytes()); OutputStream outputStream = new ByteArrayOutputStream(248); + CountDownLatch closedLatch = new CountDownLatch(1); Review Comment: nit: for this, `OneShotLatch` is available. But `CountDownLatch(1)` does the same ¯\_(ツ)_/¯ -- 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: issues-unsubscr...@flink.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org