XComp commented on code in PR #21467:
URL: https://github.com/apache/flink/pull/21467#discussion_r1060508615
##########
flink-runtime/src/main/java/org/apache/flink/runtime/heartbeat/DefaultHeartbeatMonitor.java:
##########
@@ -35,9 +36,9 @@
*
* @param <O> Type of the payload being sent to the associated heartbeat target
*/
-public class HeartbeatMonitorImpl<O> implements HeartbeatMonitor<O>, Runnable {
+public class DefaultHeartbeatMonitor<O> implements HeartbeatMonitor<O>,
Runnable {
Review Comment:
> One small suggestion, can we rename HeartbeatMonitorImpl to
DefaultHeartbeatMonitor to keep it consistent with other coordination
interfaces?
Just my 2ct on that one: Do we have a common scheme for that one. Initially,
I thought of commenting on `HeartbeatMonitorImpl` as well because I'm used to
the `Default*` naming approach historically. But I noticed that even in
`flink-runtime` we use `*Impl` quite often. I couldn't find anything in
[Flink's coding
conventions](https://flink.apache.org/contributing/code-style-and-quality-common.html).
My impression is that other modules like `flink-streaming-java` make use of
the `*Impl` scheme quite a bit.
```
$ cd ./flink
$ grep --include="*java" -Hrn "class [A-Za-z]*Impl " . | wc -l
154
$ grep --include="*java" -Hrn "class Default[A-Za-z]* " . | wc -l
176
```
It doesn't look like there's a clear winner. I'm just curious on your view
on that one.
##########
flink-core/src/main/java/org/apache/flink/configuration/HeartbeatManagerOptions.java:
##########
@@ -50,6 +50,8 @@ public class HeartbeatManagerOptions {
private static final String HEARTBEAT_RPC_FAILURE_THRESHOLD_KEY =
"heartbeat.rpc-failure-threshold";
+ public static final int FAILED_RPC_DETECTION_DISABLED = -1;
Review Comment:
hm, I'm not feeling comfortable with adding production code (which I
consider the default value to be) to the options definitions. The reason for
this change was that flink-core doesn't have access to flink-runtime, wasn't
it? Alternatively, we could move the HeartbeatManagerOptions into the
flink-runtime package. That would require us to add the package that contains
the options to
[ConfigurationOptionLocator](https://github.com/apache/flink/blob/6cce68dcdc1baf4be2a9e1549983d010644b5ee3/flink-docs/src/main/java/org/apache/flink/docs/util/ConfigurationOptionLocator.java).
I understand that this is a more invasive change. But it feels more natural
to have the options where the actual implementation is, anyway. WDYT?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
