Skip to content
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

topsql: introduce pub/sub mode #29020

Closed
wants to merge 60 commits into from
Closed

topsql: introduce pub/sub mode #29020

wants to merge 60 commits into from

Conversation

zhongzc
Copy link
Contributor

@zhongzc zhongzc commented Oct 21, 2021

What problem does this PR solve?

Make the diagnostic node dependent on TiDB instead of TiDB dependent on the diagnostic node to reverse the dependency to a normal form.

Depends on: pingcap/tipb#242
Related: tikv/tikv#11087

What is changed and how it works?

  • Global variable tidb_enable_top_sql doesn't affect whether to enable topsql or not on current instance directly.

  • Provide gRPC service tipb.TopSQLPubSubServer

    • Clients can subscribe TopSQL data through this service
  • Modify interface ReportClient

    • Rename to DataSink
    • Add two methods: IsPaused, IsDown. Reporter uses them to enable/disable TopSQL.
    • Add a new implementation pubSubDataSink for connecting the reporter and external subscribers

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory

Release note

Add TopSQL pub/sub gRPC service for publishing TopSQL data

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 21, 2021
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 25, 2021
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2021
@zhongzc zhongzc marked this pull request as ready for review October 26, 2021 03:32
@zhongzc zhongzc requested a review from a team as a code owner October 26, 2021 03:32
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2021
@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. labels Oct 26, 2021
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
Copy link
Contributor Author

zhongzc commented Oct 26, 2021

/cc @crazycs520 @mornyx @breeswish

@ti-chi-bot
Copy link
Member

@zhongzc: GitHub didn't allow me to request PR reviews from the following users: mornyx.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @crazycs520 @mornyx @breeswish

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 kubernetes/test-infra repository.

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
util/topsql/topsql.go Outdated Show resolved Hide resolved
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@zhongzc
Copy link
Contributor Author

zhongzc commented Dec 10, 2021

/run-unit-test

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
zhongzc and others added 2 commits December 13, 2021 13:01
Co-authored-by: crazycs <crazycs520@gmail.com>
Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@@ -57,6 +57,9 @@ func TestMain(m *testing.M) {
goleak.IgnoreTopFunction("go.etcd.io/etcd/pkg/logutil.(*MergeLogger).outputLoop"),
goleak.IgnoreTopFunction("go.opencensus.io/stats/view.(*worker).start"),
goleak.IgnoreTopFunction("gopkg.in/natefinch/lumberjack%2ev2.(*Logger).millRun"),
goleak.IgnoreTopFunction("time.Sleep"),
goleak.IgnoreTopFunction("runtime/pprof.readProfile"),
goleak.IgnoreTopFunction("github.com/pingcap/tidb/util/topsql/tracecpu.(*sqlCPUProfiler).startAnalyzeProfileWorker"),
Copy link
Member

@breezewish breezewish Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it may not be accepted to let this function leak.

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
Comment on lines +54 to +58
// EnablePProfSQLCPU is true means want to keep the `sql` label. Otherwise, remove the `sql` label.
EnablePProfSQLCPU = atomic.NewBool(false)

// PrecisionSeconds indicates profile interval.
PrecisionSeconds = atomic.NewInt64(DefPrecisionSeconds)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should be a config of sqlCPUProfiler I think, from this module's perspective.

}
}, nil)

<-ctx.Done()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are waiting goroutine to finish, why not execute it in current goroutine directly?

Copy link
Contributor Author

@zhongzc zhongzc Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That spawned goroutine may block when the deadline is exceeded, in that case, this datasink should be closed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. You can add a comment to document the reason.

func (sp *sqlCPUProfiler) IsEnabled() bool {
return variable.TopSQLEnabled() || sp.hasExportProfileTask()
// ShouldProfile return true if it's required to profile. It exports for tests.
func (sp *sqlCPUProfiler) ShouldProfile() bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it is not checking the enabling state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which enabling state?

Signed-off-by: Zhenchi <zhongzc_arch@outlook.com>
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2021
@ti-chi-bot
Copy link
Member

@zhongzc: PR needs rebase.

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 kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants