Skip to content

limit size of X-Druid-Response-Context header to 7K#2336

Merged
himanshug merged 1 commit intoapache:masterfrom
himanshug:fix_2331
Feb 2, 2016
Merged

limit size of X-Druid-Response-Context header to 7K#2336
himanshug merged 1 commit intoapache:masterfrom
himanshug:fix_2331

Conversation

@himanshug
Copy link
Copy Markdown
Contributor

fixes #2331

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The limit is probably on the bytes rather than chars so I think it would be better to writeValueAsBytes

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

header bytes are encoded using ASCII, so 1 char = 1 byte
in the builder.header(String key, Object value), eventually somewhere value.toString() is called and encoded using ASCII . I tried passing a byte[] to builder.header(..) and in response I got [B@468a1ce3.
I have updated the comment.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringUtils.toUtf8(responseCtxString).length might not equal responseCtxString.length()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens in utf8 multi-byte characters in headers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@drcrallen @himanshug the most correct way would be to enable the JsonGenerator .Feature.ESCAPE_NON_ASCII feature in jackson to ensure we correctly escape all non-ascii characters, otherwise the resulting context may not be valid JSON.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xvrl i guess if we are worried about invalid json in the header, then we should really move towards having a first class "metadata" attribute in the query response payload and put the information there. header is not probably the right place as you are going to remove some stuff to make it valid json anyway.

@fjy fjy added this to the 0.9.0 milestone Jan 27, 2016
@drcrallen drcrallen added the Bug label Jan 27, 2016
@xvrl
Copy link
Copy Markdown
Member

xvrl commented Jan 27, 2016

@himanshug added a few more comments that got buried in the rebase, in case you don't see them.

#2336 (comment)
#2336 (comment)
#2336 (comment)

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Feb 1, 2016

👍

@xvrl
Copy link
Copy Markdown
Member

xvrl commented Feb 2, 2016

@himanshug we don't have to get held up by the non-ascii stuff, but I feel having a header that might either be incomplete or mangled will have to be addressed at some point.

+1 once we file an issue about the non-ascii mangling in reponse context

himanshug added a commit that referenced this pull request Feb 2, 2016
limit size of X-Druid-Response-Context header to 7K
@himanshug himanshug merged commit dc89cdd into apache:master Feb 2, 2016
@himanshug
Copy link
Copy Markdown
Contributor Author

@xvrl tracking in #2372

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

introduce mechanism to limit response context size

5 participants