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

config: add warning, when users set configuration to mydumper/loader/syncer b… #1410

Merged
merged 12 commits into from
Feb 2, 2021
Merged

Conversation

zeminzhou
Copy link
Contributor

@zeminzhou zeminzhou commented Jan 29, 2021

…ut not use them

What problem does this PR solve?

For some configurations like routes/filters/mydumpers/loaders/syncers that users set in global configuration, this pr add “reference count” for these configurations.

What is changed and how it works?

When some configurations are set in global configuration, but instances don't use thme. An error will return.

Tests

  • unit test

@lance6716 lance6716 changed the title Add warning, when users set configuration to mydumper/loader/syncer b… config: add warning, when users set configuration to mydumper/loader/syncer b… Jan 29, 2021
@lance6716 lance6716 added this to the v2.0.2 milestone Jan 29, 2021
@lance6716 lance6716 added needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Jan 29, 2021
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

I don't think users will check dm-worker's log when they start a task. I think we should print these messages to result.Msg or something else.

@zeminzhou
Copy link
Contributor Author

I don't think users will check dm-worker's log when they start a task. I think we should print these messages to result.Msg or something else.

good idea

dm/config/task.go Outdated Show resolved Hide resolved
dm/config/task.go Outdated Show resolved Hide resolved
dm/config/task.go Outdated Show resolved Hide resolved
dm/config/task_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@lance6716 lance6716 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/terror/error_list.go Outdated Show resolved Hide resolved
dm/config/task.go Outdated Show resolved Hide resolved
@lance6716
Copy link
Collaborator

for failed tests, you could sort the slice to get the stable result. rest LGTM

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

/lgtm

dm/config/task_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

LGTM

@lance6716
Copy link
Collaborator

/lgtm

@ti-srebot ti-srebot added the status/LGT1 One reviewer already commented LGTM label Feb 2, 2021
@lance6716 lance6716 merged commit a0af040 into pingcap:master Feb 2, 2021
ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Feb 2, 2021
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link

cherry pick to release-2.0 in PR #1415

@ti-srebot ti-srebot added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 labels Feb 2, 2021
lance6716 pushed a commit that referenced this pull request Feb 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked first-time-contributor needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/LGT1 One reviewer already commented LGTM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants