Core: Use Stream overload for reading response in HTTPClient#15648
Core: Use Stream overload for reading response in HTTPClient#15648alpbeysir wants to merge 3 commits intoapache:mainfrom
Conversation
|
@alpbeysir can you please share how the stacktrace + allocations look like with the fix? |
|
@nastra yep here it is |
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
@danielcweeks or @amogh-jahagirdar can you guys also take a look at this please?
| if (parserContext != null && !parserContext.isEmpty()) { | ||
| reader = reader.with(parserContext.toInjectableValues()); | ||
| } | ||
| return reader.readValue(response.getEntity().getContent()); |
There was a problem hiding this comment.
Looks like Jackson takes the responsibility here of closing the input stream internally (which is fine and expected). Just wanted to make sure this doesn't break any assumptions at the HTTPClient level around connection reuse but don't think it does , so I think this solution is strictly better!
| } | ||
|
|
||
| if (responseBody == null) { | ||
| if (response.getEntity() == null) { |
There was a problem hiding this comment.
Are we sure this is the right behavior? There are a number of valid return codes (e.g. 204) that I believe result in a null entity, but are valid.
I'm a little concerned that we're not properly handling all response codes.
There was a problem hiding this comment.
I think before extractResponseBodyAsString(CloseableHttpResponse response) would also call getEntity() and do the same check so should be ok now
There was a problem hiding this comment.
There was a problem hiding this comment.
would fail here on main:
5 participants
For tables with big metadata files
loadTablewas allocating a lot ofchar[]