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

mydumper, worker: don't auto-resume when can't acquire global lock #857

Merged
merged 11 commits into from
Aug 5, 2020

Conversation

lance6716
Copy link
Collaborator

@lance6716 lance6716 commented Aug 4, 2020

What problem does this PR solve?

close #837

What is changed and how it works?

add a terror and check it in isResumableError

Check List

Tests

  • Integration test

Code changes

Side effects

Related changes

- need cherry-pick terror to master

@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 Aug 4, 2020
@csuzhangxc
Copy link
Member

NOTE: should this terror also be added in master, to avoid future conflict?

YES. all errors in all branches should always be the same, and in fact, before releasing any version, we must ensure errros codes have synced between branches.

@csuzhangxc
Copy link
Member

BTW, does the master branch (with the newest dumpling) has this problem?

@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #857 into release-1.0 will decrease coverage by 0.0137%.
The diff coverage is 15.3846%.

@@                 Coverage Diff                 @@
##           release-1.0       #857        +/-   ##
===================================================
- Coverage      57.6815%   57.6677%   -0.0138%     
===================================================
  Files              168        168                
  Lines            16761      16765         +4     
===================================================
  Hits              9668       9668                
- Misses            6210       6214         +4     
  Partials           883        883                

@lance6716
Copy link
Collaborator Author

BTW, does the master branch (with the newest dumpling) has this problem?

if this fail is caused by lack of RELOAD privilege, dumpling could keep pause.

"Error 1227: Access denied; you need (at least one of) the RELOAD privilege(s) for this operation"

but we can't simulate other fail reason

@csuzhangxc
Copy link
Member

BTW, does the master branch (with the newest dumpling) has this problem?

if this fail is caused by lack of RELOAD privilege, dumpling could keep pause.

"Error 1227: Access denied; you need (at least one of) the RELOAD privilege(s) for this operation"

but we can't simulate other fail reason

OK, we can only fix it in release-1.0 now.

@csuzhangxc csuzhangxc added this to the v1.0.7 milestone Aug 4, 2020
pkg/terror/error_list.go Outdated Show resolved Hide resolved
check_log_contains $WORK_DIR/worker2/log/dm-worker.log "Couldn't acquire global lock"
run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \
"query-status test" \
"\"stage\": \"Paused\"" 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

check error message and workaround here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed in 8c017f5

@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 Aug 4, 2020
mydumper/mydumper.go Outdated Show resolved Hide resolved
mydumper/mydumper.go Outdated Show resolved Hide resolved
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 Aug 5, 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.

LGTM

@GMHDBJD GMHDBJD added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Aug 5, 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 Aug 5, 2020
@lance6716 lance6716 merged commit 8de7dcc into pingcap:release-1.0 Aug 5, 2020
@lance6716 lance6716 deleted the fix837 branch August 5, 2020 07:40
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