zhuzhurk commented on code in PR #22506:
URL: https://github.com/apache/flink/pull/22506#discussion_r1191985909
##########
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/failover/flip1/ExecutionFailureHandler.java:
##########
@@ -121,6 +122,7 @@ private FailureHandlingResult handleFailure(
failedExecution,
new JobException("The failure is not recoverable", cause),
timestamp,
+ FailureEnricherUtils.EMPTY_LABELS,
globalFailure);
}
Review Comment:
How about to do failure labeling in this method, all failures of
DefaultScheduler will go through this path. And extra benefits are:
1. FailureHandlingResult can host all kinds of labels, no matter it's global
or task failure.
2. Can simplify the modification of Execution. No need to modify
fail()/markFailed()/etc.
3. No need to change TaskExecutionStateTransition to temporarily host the
labels.
##########
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptivebatch/AdaptiveBatchScheduler.java:
##########
@@ -113,14 +115,15 @@ public AdaptiveBatchScheduler(
final CheckpointRecoveryFactory checkpointRecoveryFactory,
final JobManagerJobMetricGroup jobManagerJobMetricGroup,
final SchedulingStrategyFactory schedulingStrategyFactory,
- final FailoverStrategy.Factory failoverStrategyFactory,
+ final Factory failoverStrategyFactory,
Review Comment:
I feel that it may make to harder to understand by making this change. If we
want to make this change, it's better to factor the `Factory` out of
`FailoverStrategy` and rename it to `FailoverStrategyFactory`.
However, that may break current custom implementations of
`FailoverStrategy.Factory`. Although it is not a public interface of Flink, I
prefer to keep it as is if the change does not bring much benefits.
##########
flink-runtime/src/test/java/org/apache/flink/runtime/executiongraph/DefaultExecutionGraphDeploymentTest.java:
##########
@@ -366,7 +367,7 @@ void testAccumulatorsAndMetricsForwarding() throws
Exception {
/**
* Verifies that {@link Execution#completeCancelling(Map, IOMetrics,
boolean)} and {@link
- * Execution#markFailed(Throwable, boolean, Map, IOMetrics, boolean,
boolean)} store the given
+ * Execution#markFailed(ErrorInfo, boolean, Map, IOMetrics, boolean,
boolean)} store the given
Review Comment:
Seems to be a mistake?
--
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]
