[FLINK-33095] Job jar issue should be reported as BAD_REQUEST instead of INTERNAL_SERVER_ERROR. #23428
Conversation
| @@ -189,7 +189,11 @@ public PackagedProgram toPackagedProgram(Configuration configuration) { | |||
| .setArguments(programArgs.toArray(new String[0])) | |||
| .build(); | |||
| } catch (final ProgramInvocationException e) { | |||
| throw new CompletionException(e); | |||
| throw new CompletionException( | |||
There was a problem hiding this comment.
I'm not sure whether we assume here that the ProgramInvocationException is caused by something the user provided (which would allow the BAD_REQUEST response). PackagedProgram#extractContainedLibraries throws a ProgramInvocationException for unknown IO errors, for instance, which would be actually an internal issue, wouldn't it?
There was a problem hiding this comment.
Thanks @XComp for review.
We have a tool that uses a REST API to submit jobs, but if the user provides the wrong entry class name by mistake, Flink returns an INTERNAL_SERVER_ERROR, which can be confusing.
You bring up a valid concern regarding exceptions like those thrown by PackagedProgram#extractContainedLibraries, which might be related to internal issues. To address this, we should consider improving our error handling to distinguish between user-provided errors and internal issues. This way, we can ensure that our responses accurately reflect the nature of the problem and provide better feedback to our users.
I will try to find a better solution for this. Do you have any suggestions?
There was a problem hiding this comment.
We have a tool that uses a REST API to submit jobs, but if the user provides the wrong entry class name by mistake, Flink returns an INTERNAL_SERVER_ERROR, which can be confusing.
I'm not questioning your intention. I just want to point out that the approach you follow might not be sufficient. The error handling of the job submission is not that concise. It requires a bigger effort to make this right in my opinion.
I'm happy to help if you're interested in addressing this issue on a broader angle.
| if (throwable instanceof RestHandlerException) { | ||
| RestHandlerException rhe = (RestHandlerException) throwable; | ||
| if (throwable instanceof RestHandlerException | ||
| || throwable.getCause() instanceof RestHandlerException) { |
There was a problem hiding this comment.
That's a strange change (checking for both, the exception and the exception's cause), don't you think? It might be an indication that we should do the change somewhere else. WDYT? 🤔
There was a problem hiding this comment.
Yes, I can throw RestHandlerException directly instead as a cause of CompletionException.
When submitting a job with incorrect parameters, such as an invalid entry class, the current response is an internal server error.
To enhance the user experience and consistency, it is recommended to throw a Rest exception and return a BAD_REQUEST response code in such cases.