clintropolis commented on code in PR #18025:
URL: https://github.com/apache/druid/pull/18025#discussion_r2114519223
##########
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:
is there any reason to not leave the `response.setHeader` here just in case?
Per the javadocs, "If the header had already been set, the new value overwrites
the previous one." so it seems like it would be harmless, and we are already
doing the work, unless i'm missing something. I would leave a comment about it
for clarity to ensure that no one deletes the other place, if you do make this
change
--
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]
