flink-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From zentol <...@git.apache.org>
Subject [GitHub] flink pull request #6222: [FLINK-8785][rest] Handle JobSubmissionExceptions
Date Mon, 02 Jul 2018 12:22:06 GMT
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.


---

Mime
View raw message