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

syncer: improve performance - part 2 #597

Merged
merged 7 commits into from
Apr 14, 2020
Merged

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Apr 13, 2020

What problem does this PR solve?

improve binlog replication performance in some cases.

What is changed and how it works?

  • do not use null value as the concurrence dispatch key
  • decrease default value of queue-size from 5120 to 1024
  • add checkpoint-flush-interval config item
  • update binlogSkippedEventsTotal to skipBinlogDurationHistogram to support observe duration and update the Grafana dashboard

image

image

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported variable/fields change
  • Has persistent data change

Side effects

  • Possible performance regression
  • Increased code complexity
  • Breaking backward compatibility

Related changes

  • Need to update the documentation
  • Need to update the dm/dm-ansible
  • Need to be included in the release note

@csuzhangxc csuzhangxc added the status/DNM Do not merge, test is failing or blocked by another PR label Apr 13, 2020
@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #597 into release-1.0 will decrease coverage by 0.0086%.
The diff coverage is 89.4736%.

@@                 Coverage Diff                 @@
##           release-1.0       #597        +/-   ##
===================================================
- Coverage      57.2752%   57.2665%   -0.0087%     
===================================================
  Files              161        161                
  Lines            16721      16741        +20     
===================================================
+ Hits              9577       9587        +10     
- Misses            6200       6206         +6     
- Partials           944        948         +4     

@csuzhangxc
Copy link
Member Author

/run-all-tests

@csuzhangxc csuzhangxc changed the title DNM: add checkpoint flush interval config item syncer: improve performance - part 2 Apr 14, 2020
@csuzhangxc csuzhangxc added needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated priority/important Major change, requires approval from ≥2 primary reviewers status/PTAL This PR is ready for review. Add this label back after committing new changes type/enhancement Performance improvement or refactoring and removed status/DNM Do not merge, test is failing or blocked by another PR labels Apr 14, 2020
@csuzhangxc csuzhangxc added this to the v1.0.5 milestone Apr 14, 2020
@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @lichunzhu PTAL

Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

seems that we have too many debug log, if we are debug other issue and need to enable debug log, these logs may disturb us.

rest LGTM

@csuzhangxc
Copy link
Member Author

csuzhangxc commented Apr 14, 2020

seems that we have too many debug log, if we are debug other issue and need to enable debug log, these logs may disturb us.

@WangXiangUSTC

em... but these logs are still needed for debug (or just let them as the placeholder). maybe when debugging, we can choose to?

  1. change some of them to info or higher
  2. do not run with debug too long

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

@lichunzhu lichunzhu 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 14, 2020
@WangXiangUSTC
Copy link
Contributor

seems that we have too many debug log, if we are debug other issue and need to enable debug log, these logs may disturb us.

em... but these logs are still needed for debug (or just let them as the placeholder). maybe when debugging, we can choose to?

  1. change some of them to info or higher
  2. do not run with debug too long

OK,LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Apr 14, 2020
@csuzhangxc csuzhangxc merged commit a45ec73 into release-1.0 Apr 14, 2020
@csuzhangxc csuzhangxc deleted the checkpoint-interval branch April 14, 2020 10:14
csuzhangxc added a commit to csuzhangxc/dm that referenced this pull request Apr 15, 2020
@csuzhangxc csuzhangxc added the needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated label Apr 23, 2020
@csuzhangxc csuzhangxc added already-update-docs The docs related to this PR already updated. Add this label once the docs are updated already-update-release-note The release note is updated. Add this label once the release note is updated and removed needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Jun 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-update-docs The docs related to this PR already updated. Add this label once the docs are updated already-update-release-note The release note is updated. Add this label once the release note is updated priority/important Major change, requires approval from ≥2 primary reviewers status/LGT2 Two reviewers already commented LGTM, ready for merge type/enhancement Performance improvement or refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants