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

*: add metrics for unified read pool #6534

Merged
merged 10 commits into from
Feb 14, 2020
Merged

Conversation

sticnarf
Copy link
Contributor

@sticnarf sticnarf commented Feb 6, 2020

What have you changed?

  1. Flush storage and coprocessor metrics by tick in unified read pool.
  2. Use newer version of yatp with metrics about multilevel thread pool. Because add basic metrics of multilevel task queue yatp#27 is not merged, a patch is used here.

With these metrics we can see how the multilevel thread pool is scheduling small and big requests:

image

What is the type of the changes?

  • Improvement (a change which is an improvement to an existing feature)

How is the PR tested?

  • Manual test (add detailed scripts or steps below)

Metrics are now available.

Does this PR affect documentation (docs) or should it be mentioned in the release notes?

No

Does this PR affect tidb-ansible?

No

@sticnarf sticnarf added type/enhancement The issue or PR belongs to an enhancement. sig/coprocessor SIG: Coprocessor component/storage Component: Storage, Scheduler, etc. labels Feb 6, 2020
@sticnarf sticnarf changed the title *: fix metrics for unified read pool *: add metrics for unified read pool Feb 6, 2020
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

Rest LGTM

cmd/src/server.rs Show resolved Hide resolved
@@ -61,3 +68,88 @@ lazy_static! {
)
.unwrap();
}

pub struct CopLocalMetrics {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not move it to src/coprocessor/local_metrics.rs?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I found src/coprocessor/local_metrics.rs is never used..

src/read_pool.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

LGTM

src/read_pool.rs Outdated
inner,
}
}

// Only flush metrics by tick
fn maybe_flush_metrics(&self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would try_flush_metrics be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

try_ usually returns a Result while here we just ignore too frequent flushes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed. PTAL again.

Copy link
Contributor

@andylokandy andylokandy left a comment

Choose a reason for hiding this comment

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

Rest LGTM

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Copy link
Contributor

@andylokandy andylokandy left a comment

Choose a reason for hiding this comment

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

LGTM

@andylokandy
Copy link
Contributor

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 14, 2020
@sre-bot
Copy link
Contributor

sre-bot commented Feb 14, 2020

/run-all-tests

@sre-bot sre-bot merged commit f20f630 into tikv:master Feb 14, 2020
c1ay pushed a commit to c1ay/tikv that referenced this pull request May 9, 2020
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/storage Component: Storage, Scheduler, etc. sig/coprocessor SIG: Coprocessor status/can-merge Indicates a PR has been approved by a committer. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants