jihoonson commented on a change in pull request #12026:
URL: https://github.com/apache/druid/pull/12026#discussion_r770186406
##########
File path:
indexing-service/src/main/java/org/apache/druid/indexing/common/task/batch/parallel/ParallelIndexSupervisorTask.java
##########
@@ -1297,7 +1298,7 @@ public Response getPhaseName(@Context final
HttpServletRequest req)
return Response.ok(runner.getName()).build();
}
} else {
- return Response.status(Status.BAD_REQUEST).entity("task is running in
the sequential mode").build();
+ return
ResponseStatusExceptionMapper.toResponse(Response.Status.BAD_REQUEST, "task is
running in the sequential mode");
Review comment:
Let me make my stance clearer here. I think
`ResponseStatusExceptionMapper` is a good idea to prevent future mistakes of
missing content type. It's worth modifying user-facing API responses. To make
sure that we always use the nice helper method instead of making a `Response`
on our own, we should probably register `Response.status()` or
`Response.serverError()` as forbidden APIs. If we are going down that way, we
should change all user-facing APIs in the same release so that we won't
introduce similar incompatible changes over multiple releases. Perhaps you or
someone else is planning to do this in multiple PRs, but because things don't
always go as planned, I think it should be better to do in one PR. But this is
going to be a huge refactoring, so I would suggest to do this separately from
the original security issue.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
