-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add more information in RequestContext class #11708
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? |
@walterddr @Jackie-Jiang can you please review? |
} | ||
|
||
@Override | ||
public void setNumConsumingSegmentsQueried(long numConsumingSegmentsQueried) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
68b52eb
to
9083b8f
Compare
9083b8f
to
b999b60
Compare
hey @walterddr can you help in reviewing this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
As per discussion in #11437 (comment) adding more information to
RequestContext
class. Most of the fields already existed inBrokerResponse
so adding more information inRequestContext
class to use in Event Listener implementation as part of #10606.