patricklucas commented on code in PR #22987:
URL: https://github.com/apache/flink/pull/22987#discussion_r1263443555
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientTest.java:
##########
@@ -207,6 +209,42 @@ public void testRestClientClosedHandling() throws
Exception {
}
}
+ /**
+ * Tests that the futures returned by {@link RestClient} fail immediately
if the client is
+ * already closed.
+ *
+ * <p>See FLINK-32583
+ */
+ @Test
+ public void testCloseClientBeforeRequest() throws Exception {
+ // Note that the executor passed to the RestClient constructor is not
the same as the
+ // executor used by Netty
+ try (final RestClient restClient =
+ new RestClient(new Configuration(),
Executors.directExecutor())) {
+ // Intentionally close the client (and thus also the executor used
by Netty)
+ restClient.close();
+
+ CompletableFuture<?> future =
+ restClient.sendRequest(
+ unroutableIp,
+ 80,
+ new TestMessageHeaders(),
+ EmptyMessageParameters.getInstance(),
+ EmptyRequestBody.getInstance());
+
+ // Call get() on the future with a timeout of 0 so we can test
that the exception thrown
+ // is not a TimeoutException, which is what would be thrown if
restClient were not
+ // already closed
+ final ThrowingRunnable getFuture = () -> future.get(0,
TimeUnit.SECONDS);
+
+ final ExecutionException executionException =
+ assertThrows(ExecutionException.class, getFuture);
+ final Throwable throwable =
ExceptionUtils.stripExecutionException(executionException);
+ assertThat(throwable, instanceOf(IOException.class));
+ assertThat(throwable.getMessage(), containsString("RestClient is
closed"));
Review Comment:
Sounds good, though the same should be true of `testConnectionTimeout` in
this same file which I followed the pattern of—do Flink contribution
expectations say I should make the same change to that test or leave it alone?
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientTest.java:
##########
@@ -207,6 +209,42 @@ public void testRestClientClosedHandling() throws
Exception {
}
}
+ /**
+ * Tests that the futures returned by {@link RestClient} fail immediately
if the client is
+ * already closed.
+ *
+ * <p>See FLINK-32583
+ */
+ @Test
+ public void testCloseClientBeforeRequest() throws Exception {
+ // Note that the executor passed to the RestClient constructor is not
the same as the
+ // executor used by Netty
+ try (final RestClient restClient =
+ new RestClient(new Configuration(),
Executors.directExecutor())) {
+ // Intentionally close the client (and thus also the executor used
by Netty)
+ restClient.close();
+
+ CompletableFuture<?> future =
+ restClient.sendRequest(
+ unroutableIp,
+ 80,
+ new TestMessageHeaders(),
+ EmptyMessageParameters.getInstance(),
+ EmptyRequestBody.getInstance());
+
+ // Call get() on the future with a timeout of 0 so we can test
that the exception thrown
+ // is not a TimeoutException, which is what would be thrown if
restClient were not
+ // already closed
+ final ThrowingRunnable getFuture = () -> future.get(0,
TimeUnit.SECONDS);
+
+ final ExecutionException executionException =
+ assertThrows(ExecutionException.class, getFuture);
+ final Throwable throwable =
ExceptionUtils.stripExecutionException(executionException);
+ assertThat(throwable, instanceOf(IOException.class));
+ assertThat(throwable.getMessage(), containsString("RestClient is
closed"));
Review Comment:
Sounds good, though the same should be true of `testConnectionTimeout` in
this same file which I followed the pattern of—do Flink contribution
expectations say I should make the same change to that test or leave it alone?
--
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
