patricklucas commented on code in PR #22987:
URL: https://github.com/apache/flink/pull/22987#discussion_r1274549521
##########
flink-runtime/src/test/java/org/apache/flink/runtime/rest/RestClientTest.java:
##########
@@ -207,6 +210,40 @@ 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 {
Review Comment:
Might be able to get the same behavior with just a `@VisibleForTesting`
getter for `responseChannelFutures`, I'll give it a shot.
Always makes me uneasy allowing injection or adding extra steps to
initializing `ConcurrentXyz` fields, even for testing, since you can't enforce
the concurrency-safety with the type system.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/RestClient.java:
##########
@@ -150,11 +159,30 @@ public static RestClient forUrl(Configuration
configuration, Executor executor,
public RestClient(Configuration configuration, Executor executor)
throws ConfigurationException {
- this(configuration, executor, null, -1);
+ this(configuration, executor, null, -1,
DefaultSelectStrategyFactory.INSTANCE);
Review Comment:
Once again, just wasn't sure what Flink's preferred style is—reducing the
number of constructor indirections or reducing the number of default values
materialized. :)
Happy to change.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/RestClient.java:
##########
@@ -150,11 +159,30 @@ public static RestClient forUrl(Configuration
configuration, Executor executor,
public RestClient(Configuration configuration, Executor executor)
throws ConfigurationException {
- this(configuration, executor, null, -1);
+ this(configuration, executor, null, -1,
DefaultSelectStrategyFactory.INSTANCE);
Review Comment:
Once again, just wasn't sure what Flink's preferred style is—reducing the
number of constructor indirections or reducing the number of default values
materialized. :)
Happy to change.
--
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
