Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6222#discussion_r199477966
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/job/JobSubmitHandler.java
---
@@ -66,6 +67,9 @@ public JobSubmitHandler(
}
return gateway.submitJob(jobGraph, timeout)
- .thenApply(ack -> new JobSubmitResponseBody("/jobs/" +
jobGraph.getJobID()));
+ .thenApply(ack -> new JobSubmitResponseBody("/jobs/" +
jobGraph.getJobID()))
+ .exceptionally(exception -> {
+ throw new CompletionException(new
RestHandlerException("Job submission failed.",
HttpResponseStatus.INTERNAL_SERVER_ERROR, exception));
--- End diff --
well, there's no doubt that it _could_ be helpful; my point is that it can
be _harmful_ if not done properly.
The `submitJob` should either provide the `JobSubmitHandler` with means to
detect these exceptions and create adequate responses, or explicitly throw
exceptions with messages that we can safely pass on to users.
That said, I do not know how to do either of these things in a good way.
ð
For completeness sake, here are ideas that came to mind:
## 1
Introduce a special `FlinkUserFacingException` that we "trust" to contain a
good error message.
Con: This provides little additional safety and will never provide proper
HTTP response code.
## 2
Introduce dedicated exceptions for the scenarios that you listed and
explicitly look for them in the `exceptionally` block, i.e
```
.exceptionally(exception -> {
if (exception instanceof JobAlreadyExistsException) {
throw new CompletionException(new RestHandlerException("Job
already exists.", HttpResponseStatus.BAD_REQUEST, exception));
} else {
throw new CompletionException(new RestHandlerException("Job
submission failed.", HttpResponseStatus.INTERNAL_SERVER_ERROR, exception));
}
}
```
Con: Obviously, this approach is inherently flawed as there is no guarantee
that a given exception can be thrown; we would have to manually keep it in sync
with the actual implementation because `CompletableFuture` throw a wrench into
sane exception handling. ð¡
## 3
Encode possible user-facing exceptions in the return value of `submitJob`,
i.e. return a `AckOrException`
```
public class AckOrException {
// holds exception, could also be a series of nullable fields
private final SuperEither<ExceptionA, ExceptionB, ExceptionC>
exception;
...
public void throwIfError() throws ExceptionA, ExceptionB, ExceptionC;
}
```
Con: Relies on users to call `throwIfError` and introduces an entirely
separate channel for passing errors, but it would allow exception matching.
---
