feat: log total bytes gathered when max scatter-gather bytes limit is reached#18841
Conversation
abhishekrb19
left a comment
abhishekrb19
left a comment
There was a problem hiding this comment.
Thanks for the PR!
It looks like jacaco is failing due to insufficient line coverage...
DirectDruidClientTest uses a bunch of mocks. I’m wondering if we could add a new test using TestHttpClient to exercise the internals of this class, including HttpResponseHandler and cover these paths.
If that turns into too much effort, I’m also fine skipping jacaco coverage for this PR since this is primarily a logging change, and we can address the test coverage debt for this class separately.
Please let me know what you think @TessaIO
cf52281 to
faebda5
Compare
|
@abhishekrb19 thanks for the suggestion, I think it would be cleaner if the test coverage is addressed in a separate PR as this one is solely focused on improving the logs. wdyt? |
faebda5 to
68b7a6e
Compare
abhishekrb19
left a comment
abhishekrb19
left a comment
There was a problem hiding this comment.
@TessaIO could you look into the CI test failures? The test client doesn't call responseContext.initialize() - we can replace the code block as follows:
protected static ResponseContext responseContext()
{
return DirectDruidClient.makeResponseContextForQuery();
}The condition maxScatterGatherBytes < Long.MAX_VALUE was previously masking this behavior.
68b7a6e to
ad827a3
Compare
|
@abhishekrb19 thanks for the review, updated! |
… reached Signed-off-by: TessaIO <[email protected]>
ad827a3 to
9a7761d
Compare
|
@abhishekrb19 done |
Test-only change: - Increases test coverage for the DirectDruidClient class from 57%, 35%, 16% to 76%, 73%, 52% for method / line / branch coverage respectively. - Replaces mocks with more concrete classes and test helpers, allowing the internals of the class, including HttpResponseHandler to be exercised. - Also added a test for feat: log total bytes gathered when max scatter-gather bytes limit is reached #18841 for which coverage had to be skipped
3 participants
Fixes #18840.
Description
log total bytes gathered when max scatter-gather bytes limit is reached
Release note
log total bytes gathered when max scatter-gather bytes limit is reached
This PR has: