ha(dm): support store task config in Etcd #3620

Ehco1996 · 2021-11-25T07:14:30Z

What problem does this PR solve?

subtask of https://github.com/pingcap/ticdc/issues/3583

What is changed and how it works?

use a new key TaskConfigTemplateKeyAdapter to store openapi.Task in etcd

Check List

Tests

Unit test

Code changes

Has exported function/method change
Has persistent data change

Side effects

Related changes

Need to update the documentation

Release note

 `support store task config template in Etcd`.

ti-chi-bot · 2021-11-25T07:14:31Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lance6716
okJiang

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

dm/dm/common/common.go

dm/openapi/task.go

dm/pkg/ha/task_config_template.go

D3Hunter

rest LGTM

dm/openapi/task.go

dm/pkg/ha/task_config_template.go

codecov-commenter · 2021-11-29T02:26:56Z

Codecov Report

Merging #3620 (91ec3cb) into master (eed6f9b) will increase coverage by 0.4309%.
The diff coverage is 71.5545%.

Flag	Coverage Δ
cdc	`59.3728% <79.4793%> (+0.9356%)`	⬆️
dm	`56.3367% <51.1811%> (-0.0386%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master      #3620        +/-   ##
================================================
+ Coverage   57.3452%   57.7761%   +0.4309%     
================================================
  Files           460        467         +7     
  Lines         54927      55632       +705     
================================================
+ Hits          31498      32142       +644     
- Misses        20180      20193        +13     
- Partials       3249       3297        +48

lance6716 · 2021-11-29T05:33:38Z

could you explain what's a task config template? for example it's constant for each version or is uploaded by web tier?

Ehco1996 · 2021-11-29T06:04:51Z

could you explain what's a task config template? for example it's constant for each version or is uploaded by web tier?

add more comments in pingcap/ticdc@c7c86c6 , is this explanation enough?

lance6716 · 2021-11-29T06:12:22Z

could you explain what's a task config template? for example it's constant for each version or is uploaded by web tier?

add more comments in c7c86c6 , is this explanation enough?

I still not understand what's a task config template. your comment doesn't answer "it's constant for each version or is uploaded by web tier?"

and it is created/updated/deleted when? the content is related to a specific DM version or user?

Ehco1996 · 2021-11-29T06:25:06Z

I still not understand what's a task config template. your comment doesn't answer "it's constant for each version or is uploaded by web tier?"

and it is created/updated/deleted when? the content is related to a specific DM version or user?

These sections don't quite fit into the comments of the code, so I'll answer you directly here

it's constant for each version or is uploaded by web tier?"

it's uploaded from openapi in WebUI

and it is created/updated/deleted when ?

the template will be created/updated/deleted when user want to operated task template in dm-cluster , and the openapi of these operations will be added in next pr, see more here

is the content is related to a specific DM version or user?

it will not releated to specific DM version or user, becasue there is only one template for each task name, if user upgrade to higher version, WebUI can auto adjust new added/deleted field then save it to etcd.

lance6716 · 2021-12-06T01:52:33Z

I remember it should be renamed to task config instead of task config template, so it's WIP now?

Ehco1996 · 2021-12-06T02:12:35Z

I remember it should be renamed to task config instead of task config template, so it's WIP now?

renamed in pingcap/ticdc@c0131e6 ptal 🧡

dm/pkg/ha/openapi_task_config.go

dm/pkg/ha/openapi_task_config_test.go

lance6716 · 2021-12-07T06:00:24Z

dm/pkg/terror/error_list.go

@@ -897,6 +899,8 @@ var (
 		"config '%s' regex pattern '%s' invalid, reason: %s", "Please check if params is correctly in the configuration file.")
 	ErrConfigOnlineDDLMistakeRegex = New(codeConfigOnlineDDLMistakeRegex, ClassConfig, ScopeInternal, LevelHigh,
 		"online ddl sql '%s' invalid, table %s fail to match '%s' online ddl regex", "Please update your `shadow-table-rules` or `trash-table-rules` in the configuration file.")
+	ErrOpenAPITaskConfigExists    = New(codeConfigOpenAPITaskConfigExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config  for '%s' already exists", "If you want to override it, pleaset use the overwite flag.")


Suggested change

ErrOpenAPITaskConfigExists = New(codeConfigOpenAPITaskConfigExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config for '%s' already exists", "If you want to override it, pleaset use the overwite flag.")

ErrOpenAPITaskConfigExists = New(codeConfigOpenAPITaskConfigExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config for '%s' already exists", "If you want to override it, pleaset use the overwite flag.")

and check typo

@Ehco1996 pleaset -> please

this sentences is genereaed by github copilot and my ide plugin does not check for syntax errors 😂

lance6716 · 2021-12-07T06:00:36Z

dm/pkg/terror/error_list.go

@@ -897,6 +899,8 @@ var (
 		"config '%s' regex pattern '%s' invalid, reason: %s", "Please check if params is correctly in the configuration file.")
 	ErrConfigOnlineDDLMistakeRegex = New(codeConfigOnlineDDLMistakeRegex, ClassConfig, ScopeInternal, LevelHigh,
 		"online ddl sql '%s' invalid, table %s fail to match '%s' online ddl regex", "Please update your `shadow-table-rules` or `trash-table-rules` in the configuration file.")
+	ErrOpenAPITaskConfigExists    = New(codeConfigOpenAPITaskConfigExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config  for '%s' already exists", "If you want to override it, pleaset use the overwite flag.")
+	ErrOpenAPITaskConfigNotExists = New(codeConfigOpenAPITaskConfigNotExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config  for '%s' not exists", "")


Suggested change

ErrOpenAPITaskConfigNotExists = New(codeConfigOpenAPITaskConfigNotExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config for '%s' not exists", "")

ErrOpenAPITaskConfigNotExists = New(codeConfigOpenAPITaskConfigNotExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config for '%s' does not exist", "")

@Ehco1996 exists -> exist

sorry fixed in pingcap/ticdc@0f81b94 and pingcap/ticdc@385e8ad

Ehco1996 · 2021-12-09T03:41:05Z

could you please take another look for this pr ? thanks 🧡🧡🧡
/cc @GMHDBJD

Ehco1996 · 2021-12-10T08:09:47Z

could you please take another look for this pr ? thanks 🧡🧡🧡

/cc @okJiang

okJiang

rest lgtm

dm/pkg/ha/openapi_task_config.go

Co-authored-by: okJiang <[email protected]>

Ehco1996 · 2021-12-13T06:43:50Z

/merge

ti-chi-bot · 2021-12-13T06:43:53Z

This pull request has been accepted and is ready to merge.

Commit hash: f9a4026

purelind · 2021-12-13T13:33:26Z

/run-integration-test

ti-chi-bot · 2021-12-13T18:08:26Z

@Ehco1996: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Ehco1996 · 2021-12-14T00:07:27Z

/run-all-tests

Ehco1996 mentioned this pull request Nov 25, 2021

Tracking issue for DM WebUI #3583

Closed

14 tasks

Ehco1996 commented Nov 25, 2021

View reviewed changes

dm/dm/common/common.go Outdated Show resolved Hide resolved

dm/openapi/task.go Show resolved Hide resolved

dm/pkg/ha/task_config_template.go Outdated Show resolved Hide resolved

Ehco1996 added the area/dm Issues or PRs related to DM. label Nov 25, 2021

Ehco1996 marked this pull request as ready for review November 25, 2021 08:54

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 25, 2021

save work

1fbfd2f

D3Hunter reviewed Nov 26, 2021

View reviewed changes

dm/openapi/task.go Show resolved Hide resolved

dm/pkg/ha/task_config_template.go Outdated Show resolved Hide resolved

address comment

0dbe465

Ehco1996 added the status/ptal Could you please take a look? label Nov 29, 2021

add PutTaskConfigTemplateIfExist

df0507a

ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 29, 2021

Merge branch 'master' into openapi-config-template

9f15048

add more comments

c7c86c6

Ehco1996 mentioned this pull request Nov 29, 2021

openapi(dm): add task config template releated openapi #3656

Merged

rename to OpenAPITaskConfig

c0131e6

lance6716 changed the title ~~ha(dm): support store task config template in Etcd~~ ha(dm): support store task config in Etcd Dec 7, 2021

lance6716 reviewed Dec 7, 2021

View reviewed changes

fix code format

698fcc2

Ehco1996 added 3 commits December 8, 2021 10:11

fix

a3da735

fix terror code name

da97ff2

Merge branch 'master' into openapi-config-template

91ec3cb

lance6716 approved these changes Dec 9, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 9, 2021

ti-chi-bot requested a review from GMHDBJD December 9, 2021 03:41

ti-chi-bot requested a review from okJiang December 10, 2021 08:09

okJiang reviewed Dec 13, 2021

View reviewed changes

dm/pkg/ha/openapi_task_config.go Outdated Show resolved Hide resolved

dm/pkg/ha/openapi_task_config.go Outdated Show resolved Hide resolved

dm/pkg/ha/openapi_task_config.go Show resolved Hide resolved

rename func

e115a8d

okJiang reviewed Dec 13, 2021

View reviewed changes

dm/pkg/ha/openapi_task_config.go Outdated Show resolved Hide resolved

Update dm/pkg/ha/openapi_task_config.go

f9a4026

Co-authored-by: okJiang <[email protected]>

okJiang approved these changes Dec 13, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 13, 2021

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 13, 2021

ti-chi-bot added 2 commits December 13, 2021 14:44

Merge branch 'master' into openapi-config-template

4e6dc64

Merge branch 'master' into openapi-config-template

c04c7a2

ti-chi-bot added 2 commits December 13, 2021 23:28

Merge branch 'master' into openapi-config-template

007e20b

Merge branch 'master' into openapi-config-template

4b28dc6

Merge branch 'master' into openapi-config-template

38df3de

ti-chi-bot merged commit 4d699cc into pingcap:master Dec 14, 2021

Ehco1996 deleted the openapi-config-template branch December 20, 2021 01:13

zhaoxinyu pushed a commit to zhaoxinyu/ticdc that referenced this pull request Dec 29, 2021

ha(dm): support store task config in Etcd (pingcap#3620)

53b3f02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ha(dm): support store task config in Etcd #3620

ha(dm): support store task config in Etcd #3620

Ehco1996 commented Nov 25, 2021

ti-chi-bot commented Nov 25, 2021 •

edited

Loading

D3Hunter left a comment

codecov-commenter commented Nov 29, 2021 •

edited

Loading

lance6716 commented Nov 29, 2021

Ehco1996 commented Nov 29, 2021

lance6716 commented Nov 29, 2021

Ehco1996 commented Nov 29, 2021

lance6716 commented Dec 6, 2021

Ehco1996 commented Dec 6, 2021

lance6716 Dec 7, 2021

lance6716 Dec 8, 2021

Ehco1996 Dec 8, 2021

lance6716 Dec 7, 2021

lance6716 Dec 8, 2021

Ehco1996 Dec 8, 2021

Ehco1996 commented Dec 9, 2021

Ehco1996 commented Dec 10, 2021

okJiang left a comment

Ehco1996 commented Dec 13, 2021

ti-chi-bot commented Dec 13, 2021

purelind commented Dec 13, 2021

ti-chi-bot commented Dec 13, 2021

Ehco1996 commented Dec 14, 2021

	ErrOpenAPITaskConfigExists = New(codeConfigOpenAPITaskConfigExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config for '%s' already exists", "If you want to override it, pleaset use the overwite flag.")
	ErrOpenAPITaskConfigExists = New(codeConfigOpenAPITaskConfigExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config for '%s' already exists", "If you want to override it, pleaset use the overwite flag.")

	ErrOpenAPITaskConfigNotExists = New(codeConfigOpenAPITaskConfigNotExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config for '%s' not exists", "")
	ErrOpenAPITaskConfigNotExists = New(codeConfigOpenAPITaskConfigNotExists, ClassConfig, ScopeInternal, LevelLow, "the openapi task config for '%s' does not exist", "")

ha(dm): support store task config in Etcd #3620

ha(dm): support store task config in Etcd #3620

Conversation

Ehco1996 commented Nov 25, 2021

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Nov 25, 2021 • edited Loading

D3Hunter left a comment

Choose a reason for hiding this comment

codecov-commenter commented Nov 29, 2021 • edited Loading

Codecov Report

lance6716 commented Nov 29, 2021

Ehco1996 commented Nov 29, 2021

lance6716 commented Nov 29, 2021

Ehco1996 commented Nov 29, 2021

lance6716 commented Dec 6, 2021

Ehco1996 commented Dec 6, 2021

lance6716 Dec 7, 2021

Choose a reason for hiding this comment

lance6716 Dec 8, 2021

Choose a reason for hiding this comment

Ehco1996 Dec 8, 2021

Choose a reason for hiding this comment

lance6716 Dec 7, 2021

Choose a reason for hiding this comment

lance6716 Dec 8, 2021

Choose a reason for hiding this comment

Ehco1996 Dec 8, 2021

Choose a reason for hiding this comment

Ehco1996 commented Dec 9, 2021

Ehco1996 commented Dec 10, 2021

okJiang left a comment

Choose a reason for hiding this comment

Ehco1996 commented Dec 13, 2021

ti-chi-bot commented Dec 13, 2021

purelind commented Dec 13, 2021

ti-chi-bot commented Dec 13, 2021

Ehco1996 commented Dec 14, 2021

ti-chi-bot commented Nov 25, 2021 •

edited

Loading

codecov-commenter commented Nov 29, 2021 •

edited

Loading