zentol commented on a change in pull request #15736:
URL: https://github.com/apache/flink/pull/15736#discussion_r619255751
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/Finished.java
##########
@@ -54,7 +54,12 @@ public ArchivedExecutionGraph getJob() {
}
@Override
- public void handleGlobalFailure(Throwable cause) {}
+ public void handleGlobalFailure(Throwable cause) {
+ logger.debug(
+ "Ignore global failure because we are in state {}.",
Review comment:
nit: Why does this follow a different pattern then the other states? I
expected something like
`"Ignored global failure because we already finished the job."`
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/Failing.java
##########
@@ -58,7 +58,7 @@ public void cancel() {
@Override
public void handleGlobalFailure(Throwable cause) {
- // nothing to do since we are already failing
+ getLogger().debug("Ignored global failure because we are already
failing the job.", cause);
Review comment:
should we maybe include the job ID?
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/Executing.java
##########
@@ -281,9 +283,9 @@ void goToFailing(
static final class FailureResult {
@Nullable private final Duration backoffTime;
- @Nullable private final Throwable failureCause;
+ private final Throwable failureCause;
- private FailureResult(@Nullable Duration backoffTime, @Nullable
Throwable failureCause) {
+ private FailureResult(Throwable failureCause, @Nullable Duration
backoffTime) {
Review comment:
Are you sure that this is safe? There are some cases where failure
causes for tasks can be null (FLINK-21376), but I'm not sure if this applies
here.
--
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]
