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

worker, utils: show same master status in one response #817

Merged
merged 12 commits into from
Jul 24, 2020

Conversation

lance6716
Copy link
Collaborator

@lance6716 lance6716 commented Jul 23, 2020

What problem does this PR solve?

fix #727

What is changed and how it works?

when one response contains multiple relay and syncer status, use the larger master binlog position overwrite them and re-check status.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

Related changes

  • Need to cherry-pick to the master branch (doing)

@lance6716 lance6716 added priority/normal Minor change, requires approval from ≥1 primary reviewer status/PTAL This PR is ready for review. Add this label back after committing new changes type/bug-fix Bug fix labels Jul 23, 2020
@lance6716 lance6716 changed the title worker, utils: show same master status worker, utils: show same master status in two continuous queries Jul 23, 2020
pkg/utils/db.go Outdated
defer masterMu.Unlock()

if useMasterStatusCache && masterStatusCached {
return masterPosCache, masterGTIDCache, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this cause global point greater than master point?

@lance6716
Copy link
Collaborator Author

lance6716 commented Jul 23, 2020 via email

@GMHDBJD
Copy link
Collaborator

GMHDBJD commented Jul 23, 2020

Yes. What's a better way? use larger master position and re-check two units?

How about use the last one master position for same worker?

@lance6716
Copy link
Collaborator Author

lance6716 commented Jul 23, 2020 via email

@GMHDBJD
Copy link
Collaborator

GMHDBJD commented Jul 23, 2020

last is larger position or something? not very understand

Yes,use the largest one

@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 23, 2020
@codecov
Copy link

codecov bot commented Jul 23, 2020

Codecov Report

Merging #817 into release-1.0 will decrease coverage by 0.0273%.
The diff coverage is 70.4545%.

@@                 Coverage Diff                 @@
##           release-1.0       #817        +/-   ##
===================================================
- Coverage      57.7345%   57.7072%   -0.0274%     
===================================================
  Files              168        168                
  Lines            16756      16757         +1     
===================================================
- Hits              9674       9670         -4     
- Misses            6200       6204         +4     
- Partials           882        883         +1     

@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 24, 2020
@lance6716 lance6716 changed the title worker, utils: show same master status in two continuous queries worker, utils: show same master status in one response Jul 24, 2020
Copy link
Collaborator

@GMHDBJD GMHDBJD 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

dm/worker/server.go Outdated Show resolved Hide resolved
@GMHDBJD
Copy link
Collaborator

GMHDBJD commented Jul 24, 2020

/run-all-tests

Copy link
Collaborator

@GMHDBJD GMHDBJD left a comment

Choose a reason for hiding this comment

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

LGTM

@GMHDBJD GMHDBJD 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 v1.0.7 milestone Jul 24, 2020
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.

rest LGTM.

Should we need to re-implement this in master branch?

dm/worker/server.go Outdated Show resolved Hide resolved
dm/worker/server_test.go Outdated Show resolved Hide resolved
@lance6716
Copy link
Collaborator Author

Should we need to re-implement this in master branch?

yes, in description of this PR I added cherry-pick to master😂, will add a "wrong" tag to remind myself

@lance6716 lance6716 added the needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 label Jul 24, 2020
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/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 275dc12 into pingcap:release-1.0 Jul 24, 2020
@lance6716 lance6716 deleted the show-one-master-status branch July 24, 2020 09:55
lance6716 added a commit to lance6716/dm that referenced this pull request Jul 25, 2020
@lance6716 lance6716 removed the needs-cherry-pick-release-1.0 This PR should be cherry-picked to release-1.0. Remove this label after cherry-picked to release-1.0 label Jul 25, 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/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants