Split thread cpu time into three metrics #7724
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7724 +/- ##
============================================
+ Coverage 64.23% 65.22% +0.99%
- Complexity 3985 4084 +99
============================================
Files 1568 1533 -35
Lines 80194 78888 -1306
Branches 11929 11792 -137
============================================
- Hits 51510 51456 -54
+ Misses 25022 23774 -1248
+ Partials 3662 3658 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
mqliang
force-pushed
the
split-metric
branch
from
75fbf15 to
0db6fea
Compare
|
LGTM thanks |
mqliang
force-pushed
the
split-metric
branch
2 times, most recently
from
b6165d2 to
114a2d7
Compare
|
PR updated:
screenshoot of offline table:
screenshoot of realtime table: screenshoot of hybrid table: |
mqliang
force-pushed
the
split-metric
branch
from
114a2d7 to
76b05e9
Compare
| LOGGER.info( | ||
| "Broker Id: {}, timeout: {}ms, query response limit: {}, query log length: {}, query log max rate: {}qps", | ||
| _brokerId, _brokerTimeoutMs, _queryResponseLimit, _queryLogLength, _queryLogRateLimiter.getRate()); | ||
| LOGGER |
There was a problem hiding this comment.
Can you please check if you import the latest Pinot Style? There are lots of reformatting changes and seems they don't follow the Pinot Style: https://docs.pinot.apache.org/developers/developers-and-contributors/code-setup#setup-ide
There was a problem hiding this comment.
@Jackie-Jiang I just double checked:
- delete the
Pinot Stylefrom myIntellj->Editor -> Code Style -> Java - import codestyle-intellij.xml from pinot/config again.
- reformat the file using the new
Pinot Style
Nothing changed. Which means the reformatting in the PR followed Pinot Style, while the old file not.
Could you please do me a favor by importing the latest Pinot Style and perform Reformat code against this file using your IntellJ (using the current master branch)?
- If your file does not change, then maybe my IntellJ have some issues, I will revert those reformatting change
- If your file also get changed, let's keep those reformatting change in the PR.
Does it sounds good?
There was a problem hiding this comment.
I actually tried that before leaving the comment and the current master file didn't change.
In the Editor -> Code Style -> Java setting, under Wrapping and Braces -> Keep when reformatting, can you uncheck the Line breaks and try reformatting again? Current setting preserves the line break when reformatting because in certain cases we want to break the line in a different way.
mqliang
force-pushed
the
split-metric
branch
from
76b05e9 to
e03d538
Compare
| * Set the total cpu time(thread cpu time + system activities cpu time + response serialization cpu time) used | ||
| * against offline table in request handling, into the broker response. | ||
| */ | ||
| void setOfflineTotalCpuTimeNs(long realtimeResponseSerializationCpuTimeNs); |
There was a problem hiding this comment.
Argument name is incorrect
| * Set the total cpu time(thread cpu time + system activities cpu time + response serialization cpu time) used | ||
| * against realtime table in request handling, into the broker response. | ||
| */ | ||
| void setRealtimeTotalCpuTimeNs(long realtimeResponseSerializationCpuTimeNs); |
|
have we measured the impact of measuring these numbers? |
We benchmarked, JVM have the stats for each thread, this measuring just retrieve the number from JVM, so the overhead is negligible. In addition, the measuring is disabled by default: |
The JVM gets the data from a clock - it delegates to clock_gettime(CLOCK_THREAD_CPUTIME_ID). Whether this call is fast or slow depends on whether the clock CLOCK_THREAD_CPUTIME_ID is in vDSO or not (which prevents a syscall) and this is platform dependent. On some platforms each call can take hundreds of nanoseconds or more, so it’s important to measure this for each deployment environment before enabling thread CPU time. |
What do you suggest? How about let's add a simple benchmark in pinot-perf. And it's user's responsibility to run the benchmark on their platform and decide if enable the thread timer. |
* Split thread CPU time into three metrics * fix typo
Description
This PR split the ThreadCpuTimeNs metric into 3 parts:
Splitting will help us understand the time spent on the 3 part separately.
screenshoot of offline table:
screenshoot of realtime table:
screenshoot of hybrid table:
cc @siddharthteotia @mcvsubbu @gshara
Upgrade Notes
Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)
backward-incompat, and complete the section below on Release Notes)Does this PR fix a zero-downtime upgrade introduced earlier?
backward-incompat, and complete the section below on Release Notes)Does this PR otherwise need attention when creating release notes? Things to consider:
release-notesand complete the section on Release Notes)Release Notes
Documentation