vahmed-hamdy commented on code in PR #29290:
URL: https://github.com/apache/flink/pull/29290#discussion_r4361472833
##########
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/CreatingExecutionGraph.java:
##########
@@ -94,16 +94,28 @@ private void handleExecutionGraphCreation(
@Nullable ExecutionGraphWithVertexParallelism
executionGraphWithVertexParallelism,
@Nullable Throwable throwable) {
if (throwable != null) {
- getLogger()
- .info(
- "Failed to go from {} to {} because the
ExecutionGraph creation failed.",
- CreatingExecutionGraph.class.getSimpleName(),
- Executing.class.getSimpleName(),
- throwable);
+ final FailureResult failureResult =
context.howToHandleEGCreationFailure(throwable);
+ if (failureResult.canRestart()) {
+ getLogger()
+ .info(
+ "Failed to create the ExecutionGraph; retrying
after {} (backoff).",
+ failureResult.getBackoffTime(),
+ throwable);
+ context.goToRetryingExecutionGraphCreation(
Review Comment:
@XComp sorry I missed that,
> Why not just trigger the existing error handling and transition to
WaitingForResources?
This is an alternative but this is not the existing pattern for job
failures, when we are backing off we will be in `CreatingExecutionGraph` before
moving back to `WaitingForResources` which is inconsistent because we are not
really creating an execution graph we are backingOff.
> That way, we could reuse existing functionality and do not need to
introduce yet another couple of config parameters. WDYT?
I think this can be done even with a new state, the choice to separate the
`RestartStrategy` was not to consume the jobs restart for EG creations + not to
enable by default but as discussed this is part of the job and can be
consolidated either ways.
--
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]
