Add more information in RequestContext class #11708
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11708 +/- ##
============================================
+ Coverage 63.04% 63.06% +0.01%
Complexity 1117 1117
============================================
Files 2342 2342
Lines 125780 125978 +198
Branches 19334 19370 +36
============================================
+ Hits 79301 79451 +150
- Misses 40828 40874 +46
- Partials 5651 5653 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 117 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@siddharthteotia @jackjlli Can you take a look? |
xiangfu0
requested review from
xiangfu0,
walterddr,
richardstartin,
Jackie-Jiang,
siddharthteotia and
jackjlli
|
@walterddr @Jackie-Jiang can you please review? |
| } | ||
|
|
||
| @Override | ||
| public void setNumConsumingSegmentsQueried(long numConsumingSegmentsQueried) { |
There was a problem hiding this comment.
(nit) I think we can use int instead of long here. May not really matter in the grand scheme of things but off late we have seen a lot of heap usage improvement opportunities from production heap dumps. So just something to consider
There was a problem hiding this comment.
this long is from BrokerResponse
| @@ -63,6 +67,19 @@ public class DefaultRequestContext implements RequestScope { | |||
|
|
|||
| private FanoutType _fanoutType; | |||
| private int _numUnavailableSegments; | |||
| private long _numConsumingSegmentsQueried; | |||
There was a problem hiding this comment.
Not directly related to this PR -- I don't think we track RequestCompilationTime in BrokerResponse or RequestContext statistics. It is emitted as a metric from the code.
But I guess we should. Within LinkedIn we have always had requests that can take tens to hundreds of ms to compile a query. Will become more relevant in the context of multi stage engine imo.
Need not necessarily do as part of this PR though.
There was a problem hiding this comment.
Yes the plan is to add more metrics as need be. Thanks for bring this up - RequestCompilationTime. I am planning to add stageStats as well for multistage queries but that needs some discussion. Will take adding RequestCompilationTime in a follow-up PR. This PR tries to get alignment between brokerResponse and RequestContext class.
| Map<String, String> getTraceInfo(); | ||
|
|
||
| void setTraceInfo(Map<String, String> traceInfo); |
There was a problem hiding this comment.
i am not sure default creating these for tracing is a good idea. normally we dont enable tracing for production use case, so for those that were manually enabled, those must've been monitored and thus renders it less useful to keep the event listener (e.g. there's a human tracking it already)
There was a problem hiding this comment.
Yes but we would still want to keep the trace-info in the event so that we can persist it in Kafka / Hive and refer it later.
tibrewalpratik17
force-pushed
the
add_more_info_request_context
branch
from
68b52eb
to
9083b8f
Compare
tibrewalpratik17
force-pushed
the
add_more_info_request_context
branch
from
9083b8f
to
b999b60
Compare
|
hey @walterddr can you help in reviewing this? |
As per discussion in #11437 (comment) adding more information to
RequestContextclass. Most of the fields already existed inBrokerResponseso adding more information inRequestContextclass to use in Event Listener implementation as part of #10606.