nicoweidner commented on a change in pull request #16165:
URL: https://github.com/apache/flink/pull/16165#discussion_r656822775
##########
File path:
flink-core/src/main/java/org/apache/flink/configuration/AkkaOptions.java
##########
@@ -39,14 +42,24 @@
+ "memory footprint.");
/** Timeout for akka ask calls. */
- public static final ConfigOption<String> ASK_TIMEOUT =
+ public static final ConfigOption<Duration> ASK_TIMEOUT_DURATION =
ConfigOptions.key("akka.ask.timeout")
- .stringType()
- .defaultValue("10 s")
+ .durationType()
+ .defaultValue(Duration.ofSeconds(10))
.withDescription(
"Timeout used for all futures and blocking Akka
calls. If Flink fails due to timeouts then you"
+ " should try to increase this value.
Timeouts can be caused by slow machines or a congested network. The"
+ " timeout value requires a time-unit
specifier (ms/s/min/h/d).");
Review comment:
Is the mention of requiring a time-unit specifier still relevant? I feel
this was important when it was string type, but it's automatically included if
the type is `Duration`
(changing it would mean that the deprecated `ASK_TIMEOUT` should have a
different description, though)
##########
File path:
flink-core/src/main/java/org/apache/flink/configuration/AkkaOptions.java
##########
@@ -101,14 +114,23 @@
"Turns on the Akka’s remote logging of events. Set
this value to 'true' in case of debugging.");
/** Timeout for all blocking calls that look up remote actors. */
- public static final ConfigOption<String> LOOKUP_TIMEOUT =
+ public static final ConfigOption<Duration> LOOKUP_TIMEOUT_DURATION =
ConfigOptions.key("akka.lookup.timeout")
- .stringType()
- .defaultValue("10 s")
+ .durationType()
+ .defaultValue(Duration.ofSeconds(10))
.withDescription(
"Timeout used for the lookup of the JobManager.
The timeout value has to contain a time-unit"
+ " specifier (ms/s/min/h/d).");
Review comment:
Same point about the time-unit specifier as for `ASK_TIMEOUT`
##########
File path:
flink-runtime/src/test/java/org/apache/flink/runtime/executiongraph/TestingDefaultExecutionGraphBuilder.java
##########
@@ -62,10 +62,9 @@ public static TestingDefaultExecutionGraphBuilder
newBuilder() {
private ScheduledExecutorService futureExecutor =
TestingUtils.defaultExecutor();
private Executor ioExecutor = TestingUtils.defaultExecutor();
- private Time rpcTimeout = AkkaUtils.getDefaultTimeout();
+ private Time rpcTimeout =
Time.fromDuration(AkkaOptions.ASK_TIMEOUT_DURATION.defaultValue());
private ClassLoader userClassLoader =
DefaultExecutionGraph.class.getClassLoader();
private BlobWriter blobWriter = VoidBlobWriter.getInstance();
- private Time allocationTimeout = AkkaUtils.getDefaultTimeout();
Review comment:
Was this field simply unused?
##########
File path:
flink-core/src/main/java/org/apache/flink/configuration/AkkaOptions.java
##########
@@ -39,14 +42,24 @@
+ "memory footprint.");
/** Timeout for akka ask calls. */
- public static final ConfigOption<String> ASK_TIMEOUT =
+ public static final ConfigOption<Duration> ASK_TIMEOUT_DURATION =
Review comment:
Looking at the usages, I wondered why we use `Duration` in the first
place instead of `Time`. I saw only two usages that actually use the Duration
class. One is in `AkkaUtils` (and could be done with `Time` as well), the other
in `WebMonitorEndpoint` (which I didn't follow further to see how it relies on
`Duration`).
Is it because `Time` is marked `@PublicEvolving`, so potentially unstable?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
