support ticdc data dir #1372

3AceShowHand · 2021-05-19T12:59:50Z

What problem does this PR solve?

For TiCDC, it might create a sort-dir to store temporary files for sort. In this PR, add data-dir to TiCDC, which can be used to store any temporary files generated by TiCDC instance.

What is changed and how it works?

add DataDir field at CDCSpec

There are two scenarios:

for version < v4.0.13, does not show data-dir in confirm information. even user set data-dir, it does not work.
for version >= v4.0.13 && version != v5.0.0-RC, show data-dir, and create it, and start CDC instance by using it.

Check List

Tests

No code

Code changes

Has exported variable/fields change

Side effects

Related changes

Need to cherry-pick to the release branch
Need to update the documentation

Release notes:

NONE

NONE

codecov-commenter · 2021-05-19T13:02:12Z

Codecov Report

Merging #1372 (c2ec4db) into master (5928416) will increase coverage by 28.86%.
The diff coverage is 56.25%.

@@             Coverage Diff             @@
##           master    #1372       +/-   ##
===========================================
+ Coverage   25.26%   54.13%   +28.86%     
===========================================
  Files         266      291       +25     
  Lines       20589    22494     +1905     
===========================================
+ Hits         5202    12177     +6975     
+ Misses      14585     8383     -6202     
- Partials      802     1934     +1132

Flag	Coverage Δ
cluster	`41.87% <56.25%> (?)`
dm	`24.46% <2.08%> (?)`
integrate	`48.70% <56.25%> (+33.19%)`	⬆️
playground	`13.73% <0.00%> (?)`
tiup	`15.49% <0.00%> (-0.02%)`	⬇️
unittest	`21.66% <8.33%> (+0.08%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/cluster/manager/check.go	`70.58% <0.00%> (+70.58%)`	⬆️
pkg/cluster/template/scripts/cdc.go	`59.09% <33.33%> (+59.09%)`	⬆️
pkg/cluster/manager/deploy.go	`71.07% <50.00%> (+71.07%)`	⬆️
pkg/cluster/manager/scale_out.go	`46.23% <60.00%> (+37.34%)`	⬆️
pkg/cluster/spec/cdc.go	`82.08% <77.77%> (+79.05%)`	⬆️
pkg/cluster/manager/upgrade.go	`59.20% <100.00%> (+53.55%)`	⬆️
pkg/cluster/spec/spec.go	`87.54% <100.00%> (+29.23%)`	⬆️
pkg/logger/logger.go	`100.00% <0.00%> (ø)`
pkg/cluster/operation/operation.go	`78.26% <0.00%> (ø)`
pkg/logger/debug.go	`16.66% <0.00%> (ø)`
... and 217 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5928416...c2ec4db. Read the comment docs.

pkg/cluster/manager/deploy.go

3AceShowHand · 2021-05-20T07:39:34Z

/run-all-tests

overvenus

Could you add some tests?

E.g., tests like

tiup/pkg/cluster/spec/spec_test.go

Lines 35 to 48 in 2faf4c9

    
           func (s *metaSuiteTopo) TestDefaultDataDir(c *C) { 
        
           	// Test with without global DataDir. 
        
           	topo := new(Specification) 
        
           	topo.TiKVServers = append(topo.TiKVServers, &TiKVSpec{Host: "1.1.1.1", Port: 22}) 
        
           	data, err := yaml.Marshal(topo) 
        
           	c.Assert(err, IsNil) 
        
           	// Check default value. 
        
           	topo = new(Specification) 
        
           	err = yaml.Unmarshal(data, topo) 
        
           	c.Assert(err, IsNil) 
        
           	c.Assert(topo.GlobalOptions.DataDir, Equals, "data") 
        
           	c.Assert(topo.TiKVServers[0].DataDir, Equals, "data")

Rest LGTM

embed/templates/scripts/run_cdc.sh.tpl

components/client/go.mod

pkg/cluster/spec/cdc.go

3AceShowHand · 2021-05-20T17:21:19Z

/run-all-tests

AstroProfundis · 2021-05-24T03:23:05Z

pkg/cluster/spec/cdc.go

 	if semver.Compare(clusterVersion, "v4.0.13") >= 0 && clusterVersion != "v5.0.0-rc" {
 		configFileSupported = true
+		dataDirSupported = true


better use a dedicated if block, so that the error message can be specific to data_dir.

and did you make sure v5.0 series versions except v5.0.0-rc support the --sort-dir argument?

yes, it's checked at the source code level.

better use a dedicated if block, so that the error message can be specific to data_dir.

I don't think we have to throw an error message on data-dir, the reason is described above.

overvenus · 2021-05-25T12:02:06Z

/lgtm

ti-chi-bot · 2021-05-25T12:02:08Z

@overvenus: Thanks for your review. The bot only counts /lgtm from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

/lgtm

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.

pkg/cluster/spec/spec.go

3AceShowHand · 2021-05-26T08:09:22Z

/run-all-tests

lucklove · 2021-05-26T09:12:06Z

/lgtm

ti-chi-bot · 2021-05-26T09:12:08Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lucklove

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

…JinLingChristopher/tiup into ling.jin/support-ticdc-data-dir

3AceShowHand · 2021-05-31T03:47:56Z

@AstroProfundis @lucklove @amyangfei @overvenus

PTAL

AstroProfundis · 2021-06-01T03:10:49Z

/merge

ti-chi-bot · 2021-06-01T03:10:52Z

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

Commit hash: c2ec4db

3AceShowHand added 3 commits May 19, 2021 19:11

add a data dir field in cdc spec.

0e0dbc2

update go mod file.

d33d050

add data dir into ticdc.

6b79d4e

ti-chi-bot requested a review from AstroProfundis May 19, 2021 13:00

ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 19, 2021

add debug log.

1421709

AstroProfundis reviewed May 20, 2021

View reviewed changes

pkg/cluster/manager/deploy.go Outdated Show resolved Hide resolved

remove debug.

b49a641

overvenus reviewed May 20, 2021

View reviewed changes

embed/templates/scripts/run_cdc.sh.tpl Outdated Show resolved Hide resolved

components/client/go.mod Show resolved Hide resolved

pkg/cluster/spec/cdc.go Show resolved Hide resolved

add some testcase for ticdc data_dir.

d77600c

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 May 20, 2021

3AceShowHand added 2 commits May 21, 2021 15:59

add data_dir by version.

6a7c430

add comment about CDCScript.WithDataDir.

6439c29

3AceShowHand requested a review from AstroProfundis May 22, 2021 15:59

AstroProfundis reviewed May 24, 2021

View reviewed changes

3AceShowHand force-pushed the ling.jin/support-ticdc-data-dir branch from 46e617d to 6439c29 Compare May 24, 2021 06:32

3AceShowHand added 4 commits May 24, 2021 16:29

set data dir to '', if cluster version lower than 4.0.13

106d7d1

fix panic on cdc data dir.

32af2e9

add debug log .

9b6294e

tiny fix on adjust.

edb5736

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 24, 2021

check version on scale out.

6704135

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2021

fix conflicts at scale-out

dabb1a3

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2021

3AceShowHand added 3 commits May 25, 2021 18:30

tiny adjust.

bd9b47a

make warn red.

c2919f9

move warn out of loop.

ea70a06

remove cdc data dir in check cluster.

d9ffcc4

lucklove reviewed May 26, 2021

View reviewed changes

pkg/cluster/spec/spec.go Outdated Show resolved Hide resolved

3AceShowHand added 2 commits May 26, 2021 11:09

add mkdir.

45191d1

remove log.

047a073

3AceShowHand requested a review from lucklove May 26, 2021 05:43

Merge branch 'master' into ling.jin/support-ticdc-data-dir

99dc83d

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

3AceShowHand added 6 commits May 26, 2021 18:59

Merge branch 'master' into ling.jin/support-ticdc-data-dir

0691dc3

Merge branch 'ling.jin/support-ticdc-data-dir' of https://github.com/…

34e2030

…JinLingChristopher/tiup into ling.jin/support-ticdc-data-dir

support data-dir .

69a1116

add testcase for cdc script initialization by version.

102370e

tiny change.

25d4b9c

fix.

940aa46

AstroProfundis added this to the v1.5.0 milestone May 28, 2021

3AceShowHand added 3 commits May 28, 2021 11:37

add ignore .

1a2bd76

Merge branch 'master' into ling.jin/support-ticdc-data-dir

476a3c5

Merge branch 'master' into ling.jin/support-ticdc-data-dir

c2ec4db

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

ti-chi-bot merged commit b12ae28 into pingcap:master Jun 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support ticdc data dir #1372

support ticdc data dir #1372

3AceShowHand commented May 19, 2021 •

edited

Loading

codecov-commenter commented May 19, 2021 •

edited

Loading

3AceShowHand commented May 20, 2021

overvenus left a comment

3AceShowHand commented May 20, 2021

AstroProfundis May 24, 2021

AstroProfundis May 24, 2021

3AceShowHand May 24, 2021

3AceShowHand May 24, 2021

overvenus commented May 25, 2021

ti-chi-bot commented May 25, 2021

3AceShowHand commented May 26, 2021

lucklove commented May 26, 2021

ti-chi-bot commented May 26, 2021

3AceShowHand commented May 31, 2021

AstroProfundis commented Jun 1, 2021

ti-chi-bot commented Jun 1, 2021


	func (s metaSuiteTopo) TestDefaultDataDir(c C) {
	// Test with without global DataDir.
	topo := new(Specification)
	topo.TiKVServers = append(topo.TiKVServers, &TiKVSpec{Host: "1.1.1.1", Port: 22})
	data, err := yaml.Marshal(topo)
	c.Assert(err, IsNil)

	// Check default value.
	topo = new(Specification)
	err = yaml.Unmarshal(data, topo)
	c.Assert(err, IsNil)
	c.Assert(topo.GlobalOptions.DataDir, Equals, "data")
	c.Assert(topo.TiKVServers[0].DataDir, Equals, "data")

support ticdc data dir #1372

support ticdc data dir #1372

Conversation

3AceShowHand commented May 19, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

codecov-commenter commented May 19, 2021 • edited Loading

Codecov Report

3AceShowHand commented May 20, 2021

overvenus left a comment

Choose a reason for hiding this comment

3AceShowHand commented May 20, 2021

AstroProfundis May 24, 2021

Choose a reason for hiding this comment

AstroProfundis May 24, 2021

Choose a reason for hiding this comment

3AceShowHand May 24, 2021

Choose a reason for hiding this comment

3AceShowHand May 24, 2021

Choose a reason for hiding this comment

overvenus commented May 25, 2021

ti-chi-bot commented May 25, 2021

3AceShowHand commented May 26, 2021

lucklove commented May 26, 2021

ti-chi-bot commented May 26, 2021

3AceShowHand commented May 31, 2021

AstroProfundis commented Jun 1, 2021

ti-chi-bot commented Jun 1, 2021

3AceShowHand commented May 19, 2021 •

edited

Loading

codecov-commenter commented May 19, 2021 •

edited

Loading