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

syncer: add metrics for sharding merge #96

Merged
merged 4 commits into from
Apr 2, 2019

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Mar 29, 2019

What problem does this PR solve?

Support to judge whether waiting for sharding DDL lock to be resolved through metrics.

What is changed and how it works?

  • add metrics for sharding merge

Check List

Tests

  • Manual test (add detailed scripts or steps below)
    1. observe dm_syncer_unsynced_table_number/dm_syncer_shard_lock_resolving
    2. execute DDL for a part of shard tables
    3. observe dm_syncer_unsynced_table_number/dm_syncer_shard_lock_resolving
    4. execute DDL for all other shard tables
    5. observe dm_syncer_unsynced_table_number/dm_syncer_shard_lock_resolving

Side effects

  • Increased code complexity

Related changes

  • Need to update the documentation
  • Need to update the dm/dm-ansible

image

@csuzhangxc csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer status/WIP This PR is still work in progress type/feature New feature labels Mar 29, 2019
@csuzhangxc csuzhangxc 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 Mar 29, 2019
@csuzhangxc
Copy link
Member Author

@GregoryIan @amyangfei PTAL

syncer/syncer.go Outdated
@@ -1574,6 +1574,8 @@ func (s *Syncer) Run(ctx context.Context) (err error) {
if err != nil {
return errors.Trace(err)
}
target, _ := GenTableID(ddlInfo.tableNames[1][0].Schema, ddlInfo.tableNames[1][0].Name)
unsyncedTableGauge.WithLabelValues(s.cfg.Name, target).Set(float64(remain))
Copy link
Collaborator

@IANTHEREAL IANTHEREAL Apr 1, 2019

Choose a reason for hiding this comment

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

why not put it into L1582 scope? and I'm afraid two many table in this metric

Copy link
Member Author

@csuzhangxc csuzhangxc Apr 1, 2019

Choose a reason for hiding this comment

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

this scope can be reached only in sharding mode, see continue in L1542.
and we obtain remain in this scope.

Copy link
Member Author

@csuzhangxc csuzhangxc Apr 1, 2019

Choose a reason for hiding this comment

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

in order to handle CreateTableStmt in L1565, so we move it into L1582 scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

addressed

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep, and we also have non-shard table in shard mode

Namespace: "dm",
Subsystem: "syncer",
Name: "shard_lock_resolving",
Help: "waiting sharding DDL lock to be resolved",
Copy link
Collaborator

Choose a reason for hiding this comment

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

shard or sharding?

@IANTHEREAL
Copy link
Collaborator

Rest LGTM

@amyangfei
Copy link
Contributor

/run-all-tests

Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@amyangfei amyangfei 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 Apr 2, 2019
@IANTHEREAL
Copy link
Collaborator

LGTM

@IANTHEREAL IANTHEREAL added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Apr 2, 2019
@csuzhangxc csuzhangxc merged commit 34e4166 into pingcap:master Apr 2, 2019
@csuzhangxc csuzhangxc deleted the shard-metrics branch April 2, 2019 11:53
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
Development

Successfully merging this pull request may close these issues.

3 participants