Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

master: add metric indicate number of pending DDL state (pessimistic only) #791

Merged
merged 12 commits into from
Jul 24, 2020
Merged

master: add metric indicate number of pending DDL state (pessimistic only) #791

merged 12 commits into from
Jul 24, 2020

Conversation

lance6716
Copy link
Collaborator

@lance6716 lance6716 commented Jul 8, 2020 •
edited
Loading

What problem does this PR solve?

part of #543. Note this PR only deal with pessimistic shard DDL lock, optimistic is more complicated and will be cover in another PR

What is changed and how it works?

show number of pending DDLs, in types of "Un-synced" and "Synced". Only leader of dm-master will take count, so we could collect from dm-masters and calculate sums to get current pending DDL number.

Check List

Tests

  • pause shardddl1 and check metrics work
    image

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

Related changes

@lance6716 lance6716 added status/WIP This PR is still work in progress priority/normal Minor change, requires approval from ≥1 primary reviewer type/feature New feature labels Jul 8, 2020
@lance6716 lance6716 mentioned this pull request Jul 8, 2020
Closed
@codecov
Copy link

codecov bot commented Jul 8, 2020 •
edited
Loading

Codecov Report

Merging #791 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #791   +/-   ##
===========================================
  Coverage   56.0734%   56.0734%           
===========================================
  Files           212        212           
  Lines         22121      22121           
===========================================
  Hits          12404      12404           
  Misses         8496       8496           
  Partials       1221       1221

@lance6716 lance6716 added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Jul 8, 2020
WangXiangUSTC
WangXiangUSTC reviewed Jul 10, 2020
View reviewed changes
dm/master/election.go Outdated
@@ -152,4 +153,5 @@ func (s *Server) retireLeader() {
s.leader = ""
s.closeLeaderClient()
s.Unlock()
metrics.RemoveDDLPendingInMetrics()
Copy link
Contributor

Choose a reason for hiding this comment

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

this metric will reset to 0 when the leader is changed, may puzzles users 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure how we supposed to use HA metrics. I think we could collect all dm-master's counter and add them, so non-leader should output zero.

Maybe we could forward metric requests to leader 😂 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

how about wait Xuecheng give some suggestion

Copy link
Member

@csuzhangxc csuzhangxc Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe PD needs to solve the same problem, can we refer to it?

or just show value for leader (introduce a role for DM-master nodes in metrics) in Grafana?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

PD use a reversed proxy (not understand) to forward every http request to leader, so this PR should be updated (but after I figure it out)

@GMHDBJD GMHDBJD added status/LGT1 One reviewer already commented LGTM status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/PTAL This PR is ready for review. Add this label back after committing new changes status/LGT1 One reviewer already commented LGTM labels Jul 14, 2020
@lance6716 lance6716 added status/WIP This PR is still work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jul 16, 2020
@lance6716
Copy link
Collaborator Author

lance6716 commented Jul 20, 2020 •
edited
Loading

seems there're many PRs editing dm.json, which need to resolve conflict such as rearrange panel, change panel ID... I prefer waiting other PR merged and then add panel for this PR.

@lance6716 lance6716 added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Jul 21, 2020
@lance6716 lance6716 changed the title master: add metric indicate number of pending DDL state master: add metric indicate number of pending DDL state (pessimistic only) Jul 21, 2020
csuzhangxc
csuzhangxc reviewed Jul 24, 2020
View reviewed changes
Copy link
Member

@csuzhangxc csuzhangxc left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Jul 24, 2020
@csuzhangxc csuzhangxc added this to the v2.0.0 RC milestone Jul 24, 2020
@csuzhangxc csuzhangxc requested a review from GMHDBJD July 24, 2020 10:25
@GMHDBJD GMHDBJD added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Jul 24, 2020
@lance6716 lance6716 merged commit a90c5f1 into pingcap:master Jul 24, 2020
@lance6716 lance6716 deleted the add-ddl-count-metric branch July 24, 2020 11:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@WangXiangUSTC WangXiangUSTC WangXiangUSTC left review comments

@csuzhangxc csuzhangxc csuzhangxc left review comments

@GMHDBJD GMHDBJD GMHDBJD approved these changes

Assignees
No one assigned
Labels
priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT2 Two reviewers already commented LGTM, ready for merge type/feature New feature
Projects
None yet
Milestone
v2.0.0 RC
Development

Successfully merging this pull request may close these issues.
4 participants
@lance6716 @csuzhangxc @WangXiangUSTC @GMHDBJD