abhishekrb19 commented on code in PR #18025:
URL: https://github.com/apache/druid/pull/18025#discussion_r2125725459
##########
server/src/main/java/org/apache/druid/server/QueryResultPusher.java:
##########
@@ -404,42 +404,10 @@ public void initialize()
DirectDruidClient.removeMagicResponseContextFields(responseContext);
- // Limit the response-context header, see
https://github.com/apache/druid/issues/2331
- // Note that Response.ResponseBuilder.header(String key,Object
value).build() calls value.toString()
- // and encodes the string using ASCII, so 1 char is = 1 byte
- ResponseContext.SerializationResult serializationResult;
- try {
- serializationResult = responseContext.serializeWith(
- jsonMapper,
- responseContextConfig.getMaxResponseContextHeaderSize()
- );
- }
- catch (JsonProcessingException e) {
- log.info(e, "Problem serializing to JSON!?");
- serializationResult = new ResponseContext.SerializationResult("Could
not serialize", "Could not serialize");
- }
-
- if (serializationResult.isTruncated()) {
- final String logToPrint = StringUtils.format(
- "Response Context truncated for id [%s]. Full context is [%s].",
- queryId,
- serializationResult.getFullResult()
- );
- if (responseContextConfig.shouldFailOnTruncatedResponseContext()) {
- log.error(logToPrint);
- throw new QueryInterruptedException(
- new TruncatedResponseContextException(
- "Serialized response context exceeds the max size[%s]",
- responseContextConfig.getMaxResponseContextHeaderSize()
- ),
- selfNode.getHostAndPortToUse()
- );
- } else {
- log.warn(logToPrint);
- }
- }
+ // validate the response context early to fail-fast, but don’t write
it to the response yet,
+ // as additional things may still be accumulated.
+ serializeAndValidateResponseContextHeader();
Review Comment:
Right, I actually made that change earlier in
https://github.com/apache/druid/pull/18025/commits/0a671f659cce152d4eeb829ff8d43c2b793929a1,
but I noticed that one of the UTs in `SqlResourceTest` started failing
[here](https://github.com/apache/druid/actions/runs/15148976352/job/42592913399)
bc there was an additional header.
Taking a closer look into the test, `SqlResourceTest` uses a
`MockHttpServletResponse` that
[uses](https://github.com/apache/druid/blob/master/server/src/test/java/org/apache/druid/server/mocks/MockHttpServletResponse.java#L58)
a `MultiMap` for `headers`, so those duplicate headers are expected with this
patch. I've updated the test to reflect that with a comment and the main code
now looks a bit cleaner I think, lmk what you think
--
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]
