Return HTTP 404 instead of 400 for supervisor/task endpoints #11724
Conversation
There was a problem hiding this comment.
This change seems reasonable. @FrankChen021 can you add some tests for this change?
Sure, I will add some tests later this week. |
There was a problem hiding this comment.
+1 from me after tests. Returning 404 instead of 400 seems more like a bug fix vs introducing an incompatible change so I'm also ok with this without a feature flag to fall back to the old behavior for any users who wouldn't want to incorporate this change on an upgrade.
Signed-off-by: frank chen <frank.chen021@outlook.com>
| @@ -386,6 +386,7 @@ public Response specGetAllHistory(@Context final HttpServletRequest req) | |||
| @GET | |||
| @Path("/{id}/history") | |||
| @Produces(MediaType.APPLICATION_JSON) | |||
| @ResourceFilters(SupervisorResourceFilter.class) | |||
There was a problem hiding this comment.
If we are using the SupervisorResourceFilter here, I think it would be safe to remove the filtering done by AuthorizationUtils.filterAuthorizedResources() later in this method?
There was a problem hiding this comment.
Thanks for pointing out this, this causes problem. And since SupervisorResourceFilter requires WRITE permission, and the function requires READ permission, I will delete the filter. I added this filter to hope the tests can have same exception message handling.
|
|
||
| import static org.easymock.EasyMock.expect; | ||
|
|
||
| public class OverlordResourceFilterTest |
There was a problem hiding this comment.
I think we should call this class TaskResourceFilterTest as TaskResourceFilter is the one used in the tests here.
| @@ -159,4 +159,6 @@ | |||
| public static final String SHUFFLE_DEEP_STORE = "shuffle-deep-store"; | |||
|
|
|||
| public static final String CUSTOM_COORDINATOR_DUTIES = "custom-coordinator-duties"; | |||
|
|
|||
| public static final String OVERLORD_RESOURCE = "overlord-resources"; | |||
There was a problem hiding this comment.
Could we use a more generic group name here that could later include other tests, say for other resources / HTTP endpoints?
There was a problem hiding this comment.
how about http-endpoint ?
| @@ -136,6 +136,9 @@ public TaskStatusPlus getTaskStatus(String taskID) | |||
| ); | |||
| return taskStatusResponse.getStatus(); | |||
| } | |||
| catch (ISE e) { | |||
There was a problem hiding this comment.
Would it make sense to catch all RuntimeExceptions here (not just ISE) and re-throw them as is?
(same comment in other places too)
There was a problem hiding this comment.
ISE is a specific exception thrown by makeRequest function when HTTP response status code is not 200. And when this exceptio occurs, its exception message contains the HTTP status code. Here we catch and re-throw the exception to simplify the exception handling to check the HTTP status code in the message.
There was a problem hiding this comment.
Okay, thanks for the clarification. 👍
|
@jihoonson Do you have any other comments? |
|
@jihoonson @asdf2014 @suneet-s @kfaraz Thank you for your review. |
Description
We're using supervisor/task HTTP APIs to manage supervisor and tasks in a independent system. Current supervisor/task endpoint return 400 when a supervisor or a task is not found. This status code is not friendly and confusing for the 3rd system. And according to the definition of HTTP status code, 404 is right code for such case. So this PR changes the status code from 400 to 404 to eliminate the ambigiuty.
This PR has: