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

*: fix context usage for SQL operation #377

Merged
merged 18 commits into from
Dec 9, 2019
Merged

*: fix context usage for SQL operation #377

merged 18 commits into from
Dec 9, 2019

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Nov 27, 2019 •
edited
Loading

What problem does this PR solve?

when retrying SQL operation to downstream, stop-task / pause-task may not take effect because of the misuse of context.

What is changed and how it works?

fix context usage for SQL operation to downstream, including:

  • data migration
  • checkpoint update
  • online DDL meta update

NOTE:

  • shard DDL meta operation not updated in this PR because we are refactoring it
  • better improvement for these operations can be done when moving them to etcd.
  • retry_cancel integration testing has much wait time, and I'll split it run separately in CI after received 2 LGTM.

Check List

Tests

  • Unit test
  • Integration test

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

@csuzhangxc csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer status/WIP This PR is still work in progress type/bug-fix Bug fix 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 already-update-release-note The release note is updated. Add this label once the release note is updated labels Nov 27, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

1 similar comment
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc csuzhangxc added needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated and removed already-update-release-note The release note is updated. Add this label once the release note is updated labels Nov 28, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@codecov
Copy link

codecov bot commented Nov 28, 2019 •
edited
Loading

Codecov Report

Merging #377 into master will increase coverage by 0.0243%.
The diff coverage is 25.8964%.

@@               Coverage Diff                @@
##             master       #377        +/-   ##
================================================
+ Coverage   57.6207%   57.6451%   +0.0244%     
================================================
  Files           163        163                
  Lines         16442      16468        +26     
================================================
+ Hits           9474       9493        +19     
- Misses         6047       6052         +5     
- Partials        921        923         +2

@csuzhangxc csuzhangxc added this to the v1.0.3 milestone Nov 28, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@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 Nov 29, 2019
@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @GregoryIan PTAL

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @GregoryIan PTAL again.

NOTE: I do not rename tctx in syncer to logCtx now because there are many places using it, and I think we should update them one by one later.

@IANTHEREAL
Copy link
Collaborator

LGTM

@IANTHEREAL
Copy link
Collaborator

In addition to the db operation, are there any operations that will block dm-ctl operation?

@IANTHEREAL
Copy link
Collaborator

I think we should add a chaos case to verify this pr, make sure it has enough full and incremental data and run long enough, while doing various operations(stop-task, pause-task, query-task) on it

@IANTHEREAL IANTHEREAL added the status/DNM Do not merge, test is failing or blocked by another PR label Dec 4, 2019
WangXiangUSTC
WangXiangUSTC reviewed Dec 4, 2019
View reviewed changes
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.

rest LGTM

loader/checkpoint.go
@@ -349,7 +347,12 @@ func (cp *RemoteCheckPoint) Count() (int, error) {
}

func (cp *RemoteCheckPoint) String() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can refine this function later, it used to log, but need visit the database, maybe we can just use information saved in RemoteCheckPoint

Copy link
Member Author

Choose a reason for hiding this comment

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

agree.

syncer/syncer.go Outdated
err := s.checkpoint.FlushPointsExcept(exceptTables, shardMetaSQLs, shardMetaArgs)

// when canceling (stop-task/pause-task), we still need to flush the checkpoint.
timeout, err := time.ParseDuration(maxDMLConnectionTimeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

we can define a new variable with time.Duration type, and then don't need parse every time.

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 in f59a6aa.

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

1 similar comment
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@WangXiangUSTC
Copy link
Contributor

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Dec 4, 2019
@csuzhangxc csuzhangxc mentioned this pull request Dec 4, 2019
Merged
@csuzhangxc csuzhangxc added already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked and removed 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 labels Dec 9, 2019
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

2 similar comments
@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc
Copy link
Member Author

/run-all-tests tidb=release-3.0

@csuzhangxc csuzhangxc merged commit 31cbed9 into pingcap:master Dec 9, 2019
@csuzhangxc csuzhangxc deleted the cancel-retry branch December 9, 2019 06:25
@csuzhangxc csuzhangxc removed the status/DNM Do not merge, test is failing or blocked by another PR label Dec 9, 2019
csuzhangxc added a commit that referenced this pull request Dec 9, 2019
@csuzhangxc
*: fix context usage for SQL operation (#377) (#400)
55ab839
@csuzhangxc csuzhangxc mentioned this pull request Dec 12, 2019
Merged
@csuzhangxc csuzhangxc added already-update-release-note The release note is updated. Add this label once the release note is updated and removed needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Mar 13, 2020
lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
@csuzhangxc
*: fix context usage for SQL operation (pingcap#377)
fb3aa2a
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IANTHEREAL IANTHEREAL IANTHEREAL left review comments

@WangXiangUSTC WangXiangUSTC WangXiangUSTC approved these changes

Assignees
No one assigned
Labels
already-cherry-pick-1.0 The related PR is already cherry-picked to release-1.0. Add this label once the PR is cherry-picked already-update-release-note The release note is updated. Add this label once the release note is updated 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
Milestone
v1.0.3
Development

Successfully merging this pull request may close these issues.
3 participants
@csuzhangxc @IANTHEREAL @WangXiangUSTC