statistics: support merge global topn in concurrency #38358
Conversation
Signed-off-by: yisaer <disxiaofei@163.com>
Signed-off-by: yisaer <disxiaofei@163.com>
|
[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
needs-cherry-pick-release-5.3
Yisaer
force-pushed
the
concurrency_merge_topn
branch
from
73dfa49
to
187a6a2
Compare
| @@ -579,6 +583,103 @@ func (h *Handle) mergePartitionStats2GlobalStats(sc sessionctx.Context, | |||
| return | |||
| } | |||
|
|
|||
| func (h *Handle) mergeGlobalStatsTopN(sc sessionctx.Context, wrapper *statistics.StatsWrapper, | |||
There was a problem hiding this comment.
Should we handle the version at first? For example, if version == 1 just return.
There was a problem hiding this comment.
Will statistics version affect this?
There was a problem hiding this comment.
Only the version2 has the TopN. Maybe for the Version1, we don't need to merge topN?
| for i, removeTopn := range resp.RemoveVals { | ||
| // Remove the value from the Hists. | ||
| if len(removeTopn) > 0 { | ||
| tmp := removeTopn | ||
| slices.SortFunc(tmp, func(i, j statistics.TopNMeta) bool { | ||
| cmpResult := bytes.Compare(i.Encoded, j.Encoded) | ||
| return cmpResult < 0 | ||
| }) | ||
| wrapper.AllHg[i].RemoveVals(tmp) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we gather all of the removeTopN for one partition from all resps. Remove them in histogram at once. Can this improve the histogram's accurate?
There was a problem hiding this comment.
The reason why I gather all the removed topn from response is to keep only reading stats during worker in order to avoid data race.
Signed-off-by: yisaer <disxiaofei@163.com>
statistics/handle/handle.go
Outdated
| start = end | ||
| } | ||
| taskNum := len(tasks) | ||
| wg := &sync.WaitGroup{} |
There was a problem hiding this comment.
replace with util.WaitGroupWrapper. We will be able to metrics those goroutine in the future.
| if len(wrapper.AllTopN) < mergeConcurrency { | ||
| mergeConcurrency = len(wrapper.AllTopN) | ||
| } | ||
| tasks := make([]*statistics.TopnStatsMergeTask, 0) |
There was a problem hiding this comment.
Maybe the capacity can be Len(wrapper.AllTopN) + 1?
There was a problem hiding this comment.
I think it's not necessary
Signed-off-by: yisaer <disxiaofei@163.com>
|
/run-check-dev_2 |
ti-chi-bot
added
status/LGT2
|
/merge |
|
This pull request has been accepted and is ready to merge. Commit hash: a308188
|
ti-chi-bot
added
the
status/can-merge
|
/run-build |
|
In response to a cherrypick label: new pull request created: #38523. |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created: #38524. |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created: #38525. |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created: #38526. |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
TiDB MergeCI notify✅ Well Done! New fixed [1] after this pr merged.
|
What problem does this PR solve?
Issue Number: ref #35142
Problem Summary:
merging global topn stats is time consuming
What is changed and how it works?
This pr makes it running in concurrency
Check List
Tests
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.