dmvk commented on a change in pull request #18689:
URL: https://github.com/apache/flink/pull/18689#discussion_r803786630
##########
File path:
flink-runtime/src/main/java/org/apache/flink/runtime/scheduler/adaptive/StateWithExecutionGraph.java
##########
@@ -148,6 +162,33 @@ public Logger getLogger() {
return logger;
}
+ protected Throwable extractError(TaskExecutionStateTransition
taskExecutionStateTransition) {
Review comment:
After the change it would be used only within this class. I'd be OK with
just making it private, so it's not exposed to the `AdaptiveScheduler`. But in
general:
```
final boolean successfulUpdate =
getExecutionGraph().updateState(taskExecutionStateTransition);
if (successfulUpdate
&& taskExecutionStateTransition.getExecutionState() ==
ExecutionState.FAILED) {
final Throwable error =
taskExecutionStateTransition.getError(classLoader);
if (error == null) {
error = new FlinkException("Unknown failure cause. Probably
related to FLINK-21376.");
}
handleFailure(
Failure.createLocal(
error,
extractExecutionVertexID(taskExecutionStateTransition)));
}
return successfulUpdate;
```
is no less readable than
```
final boolean successfulUpdate =
getExecutionGraph().updateState(taskExecutionStateTransition);
if (successfulUpdate
&& taskExecutionStateTransition.getExecutionState() ==
ExecutionState.FAILED) {
handleFailure(
Failure.createLocal(
extractError(taskExecutionStateTransition),
extractExecutionVertexID(taskExecutionStateTransition)));
}
return successfulUpdate;
```
and it avoids additional noise / extra method (eg, when you read the code,
you need to go to `extractError` to see what it really does). This will become
especially visible when we get rid of the else clause.
But again, I have no strong feelings if we just change it to private.
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
