*: apply perf flags to record RocksDB perf context in coprocessor #12432
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note-none
Signed-off-by: Yilin Chen <[email protected]>
Signed-off-by: Yilin Chen <[email protected]>
|
What do you think of setting the read stats in the coprocessor response's execution details? |
Yes, they should be returned, but I'm not going to include them in this PR. We are going to add a few more execution details, not only about coprocessor read, but also about write operations. I plan to think more about the protocol before including the statistics in the response. |
| PerfContextKind::RaftstoreApply => { | ||
| report_perf_context!(self, APPLY_PERF_CONTEXT_TIME_HISTOGRAM_STATIC); | ||
| report_write_perf_context!(self, APPLY_PERF_CONTEXT_TIME_HISTOGRAM_STATIC); | ||
| } | ||
| PerfContextKind::RaftstoreStore => { | ||
| report_perf_context!(self, STORE_PERF_CONTEXT_TIME_HISTOGRAM_STATIC); | ||
| report_write_perf_context!(self, STORE_PERF_CONTEXT_TIME_HISTOGRAM_STATIC); | ||
| } | ||
| PerfContextKind::GenericRead => { | ||
| // TODO: Currently, metrics about reading is reported in other ways. | ||
| // It is better to unify how to report the perf metrics. | ||
| // | ||
| // Here we only record the PerfContext data into the fields. | ||
| self.read = ReadPerfContext::capture(); | ||
| } |
There was a problem hiding this comment.
Sorry, what do you want it to be generic about? This is under engine_rocks, so it needn't be generic over engine.
There was a problem hiding this comment.
I hadn't thought clearly. Never mind...
So we will report these statistics as metrics in the future?
There was a problem hiding this comment.
And those statistics will also return as coprocessor response...
I hope that there will be an explicit way to remind people of their multiple purposes.
There was a problem hiding this comment.
We already do. Now, it's in the coprocessor mod where we report these statistics.
But I think we can report these metrics like raftstore statistics here as well. So, it will look more consistent about reporting rocksdb statistics.
There was a problem hiding this comment.
What about changing report_metrics to stop_observe and add another trait function to PerfContext to get the inner data?
ti-chi-bot
added
status/LGT2
|
/merge |
|
@sticnarf: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
This pull request has been accepted and is ready to merge. Commit hash: ab7bb6a
|
ti-chi-bot
added
the
status/can-merge
…kv#12432) ref tikv#12362 This commit enables some more perf flags about reading, such as block read time, DB mutex wait time. So, it enables us to find out the root cause of more performance issues. It also includes some refactorings of unifying the functions of capturing PerfContext in tikv_kv into engine_rocks because engine specific functions should be hidden in engine_* as much as possible. This is the first step that removes statistics from tikv_kv, and later code referencing directly to the engine_rocks should be also removed. Signed-off-by: Yilin Chen <[email protected]> Signed-off-by: 3AceShowHand <[email protected]>
What is changed and how it works?
Issue Number: Ref #12362
What's Changed:
Check List
Tests
Side effects
I tried TPC-H and sysbench select_random_ranges and find no obvious regression.
Release note