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
