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

dump/: integrate dumpling #540

Merged
merged 26 commits into from
Mar 18, 2020
Merged

Conversation

lichunzhu
Copy link
Contributor

@lichunzhu lichunzhu commented Mar 13, 2020

What problem does this PR solve?

#526

What is changed and how it works?

Dump sql using dumpling. It's not easy to use for users because they need to download a mydumper binary file.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has exported variable/fields change
  • Has interface methods change
  • Has persistent data change

Side effects

  • Possible performance regression

Related changes

  • Need to update the documentation
  • Need to update the dm/dm-ansible
  • 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 Mar 13, 2020
@lichunzhu
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

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

@WangXiangUSTC @csuzhangxc PTAL

@lichunzhu
Copy link
Contributor Author

/run-all-tests tidb=release-3.0

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

pkg/schema/tracker.go Outdated Show resolved Hide resolved
tests/all_mode/data/db1.increment.sql Show resolved Hide resolved
tests/print_status/conf/dm-task.yaml Outdated Show resolved Hide resolved
tests/sequence_sharding/run.sh Outdated Show resolved Hide resolved
dm/worker/metrics.go Show resolved Hide resolved
dumpling/dumpling.go Outdated Show resolved Hide resolved
dumpling/dumpling.go Outdated Show resolved Hide resolved
dumpling/dumpling.go 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, but do not forget to update dumpling in go.mod after its PR merged.

@csuzhangxc csuzhangxc removed the status/PTAL This PR is ready for review. Add this label back after committing new changes label Mar 17, 2020
@csuzhangxc csuzhangxc added the status/LGT1 One reviewer already commented LGTM label Mar 17, 2020
@lichunzhu
Copy link
Contributor Author

pingcap/dumpling#51
dumpling has proposed a PR to combine logger with DM. This PR should include this feature and be merged after this PR is merged.

@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

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

@@             Coverage Diff             @@
##             master       #540   +/-   ##
===========================================
  Coverage   57.4546%   57.4546%           
===========================================
  Files           183        183           
  Lines         19337      19337           
===========================================
  Hits          11110      11110           
  Misses         7118       7118           
  Partials       1109       1109

dumpling/dumpling.go Show resolved Hide resolved
} else {
select {
case <-ctx.Done():
isCanceled = true
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the isCanceled used for?

dumpConfig.Where = cfg.Where
}

var ret []string
Copy link
Contributor

Choose a reason for hiding this comment

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

ret is used to save some unsupported arguments? how about adding a comment

dumpling/util.go Outdated Show resolved Hide resolved
dumpling/dumpling.go Outdated Show resolved Hide resolved

extraArgs := strings.Fields(cfg.ExtraArgs)
if len(extraArgs) > 0 {
ret = append(ret, ParseArgLikeBash(extraArgs)...)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to parse the extra-args, like some config in the integration test, the user may set extra-args: "--statement-size=100", it may cause incompatible problem

WangXiangUSTC
WangXiangUSTC previously approved these changes Mar 18, 2020
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 Mar 18, 2020
@lichunzhu lichunzhu merged commit 5220517 into pingcap:master Mar 18, 2020
@lichunzhu lichunzhu deleted the czli/integrateDumpling branch March 18, 2020 13:10
lichunzhu added a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Dump sql using dumpling. It's not easy to use for users because they need to download a mydumper binary file.
lichunzhu added a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020
Dump sql using dumpling. It's not easy to use for users because they need to download a mydumper binary file.
lichunzhu added a commit to lichunzhu/dm that referenced this pull request Apr 7, 2020
Dump sql using dumpling. It's not easy to use for users because they need to download a mydumper binary file.
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