loader(dm): register tls config when clean checkpoint #3522

Ehco1996 · 2021-11-18T09:01:20Z

What problem does this PR solve?

What is changed and how it works?

set right logger config when init lightning
register and deregister TLS config when removing checkpoint

Check List

Tests

Unit test
Integration test

Code changes

None

Side effects

Increased code complexity

Related changes

None

Release note

Please add a release note.
`None`.

ti-chi-bot · 2021-11-18T09:01:21Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

glorv
lance6716

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.

Ehco1996 · 2021-11-22T01:37:56Z

/run-dm-integration-tests

Ehco1996 · 2021-11-22T03:55:20Z

/run-dm-integration-tests

codecov-commenter · 2021-11-23T02:14:11Z

Codecov Report

Merging #3522 (fa145d0) into master (eed6f9b) will increase coverage by 0.4584%.
The diff coverage is 70.1877%.

Flag	Coverage Δ
cdc	`59.4059% <79.4793%> (+0.9687%)`	⬆️
dm	`56.3560% <39.6984%> (-0.0193%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #3522        +/-   ##
================================================
+ Coverage   57.3452%   57.8036%   +0.4584%     
================================================
  Files           460        465         +5     
  Lines         54927      55538       +611     
================================================
+ Hits          31498      32103       +605     
+ Misses        20180      20150        -30     
- Partials       3249       3285        +36

lance6716 · 2021-11-23T02:50:17Z

dm/loader/lightning.go

 		if err = cfg.Adjust(ctx); err != nil {
 			return err
 		}
 		err = l.runLightning(ctx, cfg)
 		if err == nil {
+			// lightning will auto deregister tls when task done, so we need to register it again for removing checkpoint


we might let lightning not deregister the certificate. If there're more than one subtasks, the certificate may still be deregister between line 245 and line 249

now lightning allow register tls config with different name, and it test is passed, ptal 🧡

CheckpointRemove should be postponed to cleanmeta. I will add this logic in a following PR, so LGTM.

Ehco1996 · 2021-11-23T03:40:56Z

/hold need change lightning behaviour and upgrade dep

glorv · 2021-12-06T03:46:56Z

dm/loader/lightning.go

 	lightningCfg.TikvImporter.Backend = cfg.TiDB.Backend
 	lightningCfg.PostRestore.Checksum = lcfg.OpLevelOff
 	if cfg.TiDB.Backend == lcfg.BackendLocal {
 		lightningCfg.TikvImporter.SortedKVDir = cfg.Dir
 	}
 	lightningCfg.Mydumper.SourceDir = cfg.Dir
+	lightningCfg.App.Config.Level = cfg.LogLevel
+	lightningCfg.App.Config.File = cfg.LogFile


We should net let lightning init log another time. pingcap/tidb#29291 this PR will skip init lightning log so lightning will share the global config with dm.

Ehco1996 · 2021-12-08T07:58:07Z

/run-all-tests

Ehco1996 · 2021-12-08T23:46:35Z

/run-all-tests

dm/loader/lightning.go

dm/tests/tls/run.sh

Ehco1996 · 2021-12-09T06:08:28Z

/run-dm-integration-tests

Ehco1996 · 2021-12-09T06:33:11Z

/run-dm-integration-tests

lance6716

rest lgtm

dm/loader/lightning.go

glorv · 2021-12-14T12:07:52Z

/merge

ti-chi-bot · 2021-12-14T12:07:56Z

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

Commit hash: fa145d0

ti-chi-bot · 2021-12-14T13:16:45Z

@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-14T23:39:16Z

/run-integration-test
/run-leak-test

dm/lightning: register tls config when clean checkpoint

ceff2c5

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

Ehco1996 added 3 commits November 18, 2021 17:05

fix lint

38032dd

fix again

3b2d9cc

fix logger config

929a355

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 19, 2021

Ehco1996 added 2 commits November 19, 2021 16:48

revert core change

0a41374

Merge branch 'master' into lightning-tls

02039f7

Ehco1996 changed the title ~~dm/lightning: register tls config when clean checkpoint~~ loader(dm): register tls config when clean checkpoint Nov 22, 2021

Merge branch 'master' into lightning-tls

2a732cc

Ehco1996 marked this pull request as ready for review November 22, 2021 01:13

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 22, 2021

Ehco1996 marked this pull request as draft November 22, 2021 03:09

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

Ehco1996 marked this pull request as ready for review November 22, 2021 06:01

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 22, 2021

Merge branch 'master' into lightning-tls

89940a8

lance6716 reviewed Nov 23, 2021

View reviewed changes

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2021

glorv reviewed Dec 6, 2021

View reviewed changes

Merge branch 'master' into lightning-tls

16df138

Ehco1996 added 2 commits December 8, 2021 14:33

clean up test dir

2b04bb6

Merge branch 'master' into lightning-tls

aead0da

Ehco1996 added status/ptal Could you please take a look? and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 8, 2021

Merge branch 'master' into lightning-tls

388bfce

lance6716 reviewed Dec 9, 2021

View reviewed changes

dm/loader/lightning.go Outdated Show resolved Hide resolved

dm/tests/tls/run.sh Outdated Show resolved Hide resolved

dm/tests/tls/run.sh Outdated Show resolved Hide resolved

Ehco1996 added 2 commits December 9, 2021 14:04

open spell checker for bash and address comment

e2b938b

revert logging postion for debug

d134f15

fix sh

4566dde

lance6716 reviewed Dec 14, 2021

View reviewed changes

dm/loader/lightning.go Outdated Show resolved Hide resolved

remove uselses lock

fa145d0

lance6716 approved these changes Dec 14, 2021

View reviewed changes

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

glorv approved these changes Dec 14, 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 14, 2021

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

ti-chi-bot added 2 commits December 14, 2021 20:08

Merge branch 'master' into lightning-tls

be7c29b

Merge branch 'master' into lightning-tls

3966613

ti-chi-bot merged commit 2057c2b into pingcap:master Dec 14, 2021

Ehco1996 deleted the lightning-tls branch December 20, 2021 01:13

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

loader(dm): register tls config when clean checkpoint (pingcap#3522)

524e26c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loader(dm): register tls config when clean checkpoint #3522

loader(dm): register tls config when clean checkpoint #3522

Ehco1996 commented Nov 18, 2021 •

edited

Loading

ti-chi-bot commented Nov 18, 2021 •

edited

Loading

Ehco1996 commented Nov 22, 2021

Ehco1996 commented Nov 22, 2021

codecov-commenter commented Nov 23, 2021 •

edited

Loading

lance6716 Nov 23, 2021

Ehco1996 Dec 8, 2021

glorv Dec 8, 2021

Ehco1996 commented Nov 23, 2021

glorv Dec 6, 2021

Ehco1996 Dec 8, 2021

Ehco1996 commented Dec 8, 2021

Ehco1996 commented Dec 8, 2021

Ehco1996 commented Dec 9, 2021

Ehco1996 commented Dec 9, 2021

lance6716 left a comment

glorv commented Dec 14, 2021

ti-chi-bot commented Dec 14, 2021

ti-chi-bot commented Dec 14, 2021

Ehco1996 commented Dec 14, 2021

loader(dm): register tls config when clean checkpoint #3522

loader(dm): register tls config when clean checkpoint #3522

Conversation

Ehco1996 commented Nov 18, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

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

Ehco1996 commented Nov 22, 2021

Ehco1996 commented Nov 22, 2021

codecov-commenter commented Nov 23, 2021 • edited Loading

Codecov Report

lance6716 Nov 23, 2021

Choose a reason for hiding this comment

Ehco1996 Dec 8, 2021

Choose a reason for hiding this comment

glorv Dec 8, 2021

Choose a reason for hiding this comment

Ehco1996 commented Nov 23, 2021

glorv Dec 6, 2021

Choose a reason for hiding this comment

Ehco1996 Dec 8, 2021

Choose a reason for hiding this comment

Ehco1996 commented Dec 8, 2021

Ehco1996 commented Dec 8, 2021

Ehco1996 commented Dec 9, 2021

Ehco1996 commented Dec 9, 2021

lance6716 left a comment

Choose a reason for hiding this comment

glorv commented Dec 14, 2021

ti-chi-bot commented Dec 14, 2021

ti-chi-bot commented Dec 14, 2021

Ehco1996 commented Dec 14, 2021

Ehco1996 commented Nov 18, 2021 •

edited

Loading

ti-chi-bot commented Nov 18, 2021 •

edited

Loading

codecov-commenter commented Nov 23, 2021 •

edited

Loading