imply-cheddar commented on code in PR #13609:
URL: https://github.com/apache/druid/pull/13609#discussion_r1055150717
##########
server/src/main/java/org/apache/druid/server/QueryResource.java:
##########
@@ -561,20 +549,27 @@ public ResultsWriter start()
{
return new ResultsWriter()
{
+ private QueryResponse<Object> queryResponse;
+
@Override
- public QueryResponse<Object> start(HttpServletResponse response)
+ public Response.ResponseBuilder start()
{
- final QueryResponse<Object> queryResponse = queryLifecycle.execute();
+ queryResponse = queryLifecycle.execute();
final ResponseContext responseContext =
queryResponse.getResponseContext();
final String prevEtag = getPreviousEtag(req);
if (prevEtag != null &&
prevEtag.equals(responseContext.getEntityTag())) {
queryLifecycle.emitLogsAndMetrics(null, req.getRemoteAddr(), -1);
counter.incrementSuccess();
- response.setStatus(HttpServletResponse.SC_NOT_MODIFIED);
- return null;
+ return Response.status(Status.NOT_MODIFIED);
Review Comment:
Returning Response from here is better in terms of reducing future code
changes if/when something else needs to return different responses. For
example, the QueryResource code, I'm pretty sure some of that could likely move
into the start() here... If/when we want to add other things, being able to
just return a ResponseBuilder is the nicest API possible.
I thought of perhaps expanding QueryResponse to have a potential
ResponseBuilder on it as well so that this could continue returning that
object. That change felt larger to me than just storing the reference and
needing to call get after start(). Either way, the interactions here are very
tight and fairly locally-scoped such that needing to call start() before
needing to call another method felt like the lesser of the evils.
--
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]
