You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "surendralilhore (via GitHub)" <gi...@apache.org> on 2023/09/16 07:55:03 UTC
[GitHub] [flink] surendralilhore opened a new pull request, #23428: [FLINK-33095] Job jar issue should be reported as BAD_REQUEST instead of INTERNAL_SERVER_ERROR.
surendralilhore opened a new pull request, #23428:
URL: https://github.com/apache/flink/pull/23428
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.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33095] Job jar issue should be reported as BAD_REQUEST instead of INTERNAL_SERVER_ERROR. [flink]
Posted by "surendralilhore (via GitHub)" <gi...@apache.org>.
surendralilhore commented on code in PR #23428:
URL: https://github.com/apache/flink/pull/23428#discussion_r1346953716
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/AbstractHandler.java:
##########
@@ -242,8 +242,14 @@ private CompletableFuture<Void> handleException(
return CompletableFuture.completedFuture(null);
}
int maxLength = flinkHttpObjectAggregator.maxContentLength() - OTHER_RESP_PAYLOAD_OVERHEAD;
- if (throwable instanceof RestHandlerException) {
- RestHandlerException rhe = (RestHandlerException) throwable;
+ if (throwable instanceof RestHandlerException
+ || throwable.getCause() instanceof RestHandlerException) {
Review Comment:
Yes, I can throw `RestHandlerException` directly instead as a cause of `CompletionException`.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33095] Job jar issue should be reported as BAD_REQUEST instead of INTERNAL_SERVER_ERROR. [flink]
Posted by "XComp (via GitHub)" <gi...@apache.org>.
XComp commented on code in PR #23428:
URL: https://github.com/apache/flink/pull/23428#discussion_r1346884026
##########
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/utils/JarHandlerUtils.java:
##########
@@ -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(
Review 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.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33095] Job jar issue should be reported as BAD_REQUEST instead of INTERNAL_SERVER_ERROR. [flink]
Posted by "XComp (via GitHub)" <gi...@apache.org>.
XComp commented on code in PR #23428:
URL: https://github.com/apache/flink/pull/23428#discussion_r1345443798
##########
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/utils/JarHandlerUtils.java:
##########
@@ -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(
Review 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](https://github.com/apache/flink/blob/c08bef1f7913eb1c416c278354fd62b82b172549/flink-clients/src/main/java/org/apache/flink/client/program/PackagedProgram.java#L554) throws a `ProgramInvocationException` for unknown IO errors, for instance, which would be actually an internal issue, wouldn't it?
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/AbstractHandler.java:
##########
@@ -242,8 +242,14 @@ private CompletableFuture<Void> handleException(
return CompletableFuture.completedFuture(null);
}
int maxLength = flinkHttpObjectAggregator.maxContentLength() - OTHER_RESP_PAYLOAD_OVERHEAD;
- if (throwable instanceof RestHandlerException) {
- RestHandlerException rhe = (RestHandlerException) throwable;
+ if (throwable instanceof RestHandlerException
+ || throwable.getCause() instanceof RestHandlerException) {
Review 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? :thinking:
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33095] Job jar issue should be reported as BAD_REQUEST instead of INTERNAL_SERVER_ERROR. [flink]
Posted by "surendralilhore (via GitHub)" <gi...@apache.org>.
surendralilhore commented on code in PR #23428:
URL: https://github.com/apache/flink/pull/23428#discussion_r1346878450
##########
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/handlers/utils/JarHandlerUtils.java:
##########
@@ -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(
Review 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#extractContainedLibrarie`s, 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?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #23428: [FLINK-33095] Job jar issue should be reported as BAD_REQUEST instead of INTERNAL_SERVER_ERROR.
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #23428:
URL: https://github.com/apache/flink/pull/23428#issuecomment-1722170441
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "87add303df04bf86b615e4b01ad865438f91292f",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "87add303df04bf86b615e4b01ad865438f91292f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 87add303df04bf86b615e4b01ad865438f91292f UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-33095] Job jar issue should be reported as BAD_REQUEST instead of INTERNAL_SERVER_ERROR. [flink]
Posted by "surendralilhore (via GitHub)" <gi...@apache.org>.
surendralilhore commented on code in PR #23428:
URL: https://github.com/apache/flink/pull/23428#discussion_r1346953716
##########
flink-runtime/src/main/java/org/apache/flink/runtime/rest/handler/AbstractHandler.java:
##########
@@ -242,8 +242,14 @@ private CompletableFuture<Void> handleException(
return CompletableFuture.completedFuture(null);
}
int maxLength = flinkHttpObjectAggregator.maxContentLength() - OTHER_RESP_PAYLOAD_OVERHEAD;
- if (throwable instanceof RestHandlerException) {
- RestHandlerException rhe = (RestHandlerException) throwable;
+ if (throwable instanceof RestHandlerException
+ || throwable.getCause() instanceof RestHandlerException) {
Review Comment:
Yes, I can throw `RestHandlerException `directly instead as a cause of `CompletionException`.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org