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

dm-ha/: add remove metadata feature #651

Merged
merged 41 commits into from
May 20, 2020
Merged

Conversation

lichunzhu
Copy link
Contributor

What problem does this PR solve?

#567
support remove-meta for dm-ha

What is changed and how it works?

  1. remove remove-meta in dm-worker.
  2. remove shard ddl etcd info, online sql info and pessimistic shard ddl info in sql on dm-master.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change

Related changes

  • Need to be included in the release note

@lichunzhu lichunzhu added priority/normal Minor change, requires approval from ≥1 primary reviewer status/WIP This PR is still work in progress labels May 7, 2020
@lichunzhu lichunzhu changed the title add remove metadata feature dm-ha/: add remove metadata feature May 7, 2020
@lichunzhu
Copy link
Contributor Author

/run-all-tests

@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #651 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #651   +/-   ##
===========================================
  Coverage   57.0840%   57.0840%           
===========================================
  Files           204        204           
  Lines         20899      20899           
===========================================
  Hits          11930      11930           
  Misses         7818       7818           
  Partials       1151       1151           

@lichunzhu lichunzhu 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 May 7, 2020
@lichunzhu
Copy link
Contributor Author

/run-all-tests

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.

how about adding test cases in integration test? for example, stop the task and then start the task?

dm/worker/join.go Outdated Show resolved Hide resolved
dm/master/server.go Outdated Show resolved Hide resolved
dm/ctl/master/start_task.go Outdated Show resolved Hide resolved
dm/proto/dmmaster.proto Outdated Show resolved Hide resolved
pkg/shardddl/pessimism/ops.go Outdated Show resolved Hide resolved
dm/master/server.go Outdated Show resolved Hide resolved
@csuzhangxc csuzhangxc added this to the v2.0.0 beta.2 milestone May 12, 2020
dm/master/shardddl/optimist.go Outdated Show resolved Hide resolved
sources = append(sources, stCfg.SourceID)
}
if req.RemoveMeta {
if scm := s.scheduler.GetSubTaskCfgsByTask(cfg.Name); len(scm) > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need any mechanism to prevent start-task in another client when removing meta?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add removeMetaLock

Copy link
Contributor

Choose a reason for hiding this comment

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

why need a lock, may start task with same task name?

Copy link
Member

Choose a reason for hiding this comment

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

em... may user start-task again when the previous start-task is still removing meta?

dm/master/server.go Outdated Show resolved Hide resolved
dm/master/server.go Outdated Show resolved Hide resolved
@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 May 15, 2020
@lichunzhu
Copy link
Contributor Author

/run-all-tests

@WangXiangUSTC
Copy link
Contributor

@lichunzhu need to resolve conflicts

@lichunzhu
Copy link
Contributor Author

@lichunzhu need to resolve conflicts

will resolve conflicts after #654 is merged at one time

@lichunzhu
Copy link
Contributor Author

@WangXiangUSTC Already resolved.

@lichunzhu
Copy link
Contributor Author

/run-all-tests

@lichunzhu
Copy link
Contributor Author

/run-all-tests

@lichunzhu
Copy link
Contributor Author

/run-all-tests

2 similar comments
@lichunzhu
Copy link
Contributor Author

/run-all-tests

@lichunzhu
Copy link
Contributor Author

/run-all-tests

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.

LGTM

@WangXiangUSTC WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels May 20, 2020
@lichunzhu lichunzhu merged commit 24b0be9 into pingcap:master May 20, 2020
@lichunzhu lichunzhu deleted the fixRemoveMeta branch May 20, 2020 07:46
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants