Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

playground: Supports TiKV-CDC component #2000

Merged
merged 14 commits into from
Sep 8, 2022

Conversation

pingyu
Copy link
Contributor

@pingyu pingyu commented Jul 27, 2022

What problem does this PR solve?

Close #1999: playground: Support component TiKV-CDC.

What is changed and how it works?

Add codes to deal with deploy, scale-in and scale-out of the TiKV-CDC component.

Check List

Tests

  • Integration test
  • Manual test (add detailed scripts or steps below)

deploy:

❯ bin/tiup-playground v6.2.0 --kvcdc 3 --kvcdc.version=v1.0.0-alpha --db 1 --kv 1 --pd 1 --tiflash 0 --tag clst30 --pd.port 20000
Playground Bootstrapping...
Start pd instance:v6.2.0
Start tikv instance:v6.2.0
Start tidb instance:v6.2.0
Start tikv-cdc instance:v1.0.0-alpha
Start tikv-cdc instance:v1.0.0-alpha
Start tikv-cdc instance:v1.0.0-alpha
Waiting for tidb instances ready
127.0.0.1:38211 ... Done
CLUSTER START SUCCESSFULLY, Enjoy it ^-^
To connect TiDB: mysql --comments --host 127.0.0.1 --port 38211 -u root -p (no password)
To view the dashboard: http://127.0.0.1:20000/dashboard
PD client endpoints: [127.0.0.1:20000]
To view the Prometheus: http://127.0.0.1:34991
To view the Grafana: http://127.0.0.1:39987
❯ 
❯ ^CPlayground receive signal:  interrupt
Wait prometheus(2794768) to quit...
Wait ng-monitoring(2794769) to quit...
Wait grafana(2794849) to quit...
Wait tikv-cdc(2793600) to quit...
Grafana quit
tikv-cdc quit
Wait tikv-cdc(2793601) to quit...
tikv-cdc quit
Wait tikv-cdc(2793611) to quit...
ng-monitoring quit
tikv-cdc quit
Wait tidb(2793599) to quit...
prometheus quit
tidb quit
Wait tikv(2793546) to quit...
tikv quit
Wait pd(2793509) to quit...
pd quit

display, scale-in and scale-out:

❯ bin/tiup-playground display --tag clst30
Pid      Role      Uptime
---      ----      ------
2670157  pd        29.357248287s
2670173  tikv      29.175895857s
2670180  tidb      29.012693018s
2670233  tikv-cdc  28.543214701s
❯ bin/tiup-playground scale-out --kvcdc 1 --tag clst30
❯ bin/tiup-playground display --tag clst30
Pid      Role      Uptime
---      ----      ------
2670157  pd        59.749018137s
2670173  tikv      59.567665054s
2670180  tidb      59.40447222s
2670233  tikv-cdc  58.934982488s
2672113  tikv-cdc  8.167426031s
❯ bin/tiup-playground scale-in --pid 2670233 --tag clst30
scale in tikv-cdc success
❯ bin/tiup-playground display --tag clst30
Pid      Role      Uptime
---      ----      ------
2670157  pd        1m22.054549013s
2670173  tikv      1m21.873198096s
2670180  tidb      1m21.709997698s
2672113  tikv-cdc  30.472933521s
❯ bin/tiup-playground scale-out --kvcdc 3 --tag clst30
❯ bin/tiup-playground display --tag clst30
Pid      Role      Uptime
---      ----      ------
2670157  pd        1m37.248401925s
2670173  tikv      1m37.067054776s
2670180  tidb      1m36.90385724s
2672113  tikv-cdc  45.666796244s
2672879  tikv-cdc  6.28220083s
2672884  tikv-cdc  6.276836013s
2672889  tikv-cdc  6.271211554s

Code changes

  • Has exported variable/fields change

Add Version to instance.Config, to keep version of TiKV-CDC that can be specified by command line argument.

// Config of the instance.
type Config struct {
	ConfigPath string `yaml:"config_path"`
	BinPath    string `yaml:"bin_path"`
	Num        int    `yaml:"num"`
	Host       string `yaml:"host"`
	Port       int    `yaml:"port"`
	UpTimeout  int    `yaml:"up_timeout"`
	Version    string `yaml:"version"`
}
  • Has command line arguments added
      --kvcdc int                TiKV-CDC instance number
      --kvcdc.binpath string     TiKV-CDC instance binary path
      --kvcdc.config string      TiKV-CDC instance configuration file
      --kvcdc.version string     TiKV-CDC instance version

Side effects

  • No.

Related changes

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

Release notes:

Playground supports the TiKV-CDC component.

pingyu added 3 commits July 26, 2022 20:12
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
Signed-off-by: pingyu <yuping@pingcap.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 27, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • AstroProfundis

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.

@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 27, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2022

Codecov Report

Base: 52.51% // Head: 46.40% // Decreases project coverage by -6.11% ⚠️

Coverage data is based on head (6f5d5a7) compared to base (42b07cb).
Patch coverage: 82.02% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2000      +/-   ##
==========================================
- Coverage   52.51%   46.40%   -6.11%     
==========================================
  Files         308      308              
  Lines       35946    36070     +124     
==========================================
- Hits        18875    16738    -2137     
- Misses      14849    17152    +2303     
+ Partials     2222     2180      -42     
Flag Coverage Δ
cluster 39.71% <ø> (-1.50%) ⬇️
tiup 14.25% <ø> (ø)
unittest ?

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

Impacted Files Coverage Δ
components/playground/instance/instance.go 48.57% <ø> (ø)
pkg/cluster/spec/instance.go 70.36% <ø> (+5.12%) ⬆️
components/playground/main.go 45.74% <56.25%> (-2.31%) ⬇️
components/playground/instance/tikv_cdc.go 85.00% <85.00%> (ø)
components/playground/playground.go 44.27% <90.00%> (-5.10%) ⬇️
components/playground/command.go 79.59% <100.00%> (+0.43%) ⬆️
components/dm/ansible/worker.go 0.00% <0.00%> (-100.00%) ⬇️
...onents/playground/instance/tiflash_proxy_config.go 0.00% <0.00%> (-80.00%) ⬇️
pkg/meta/err.go 0.00% <0.00%> (-76.19%) ⬇️
pkg/cluster/api/error.go 0.00% <0.00%> (-75.00%) ⬇️
... and 71 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

if c.ConfigPath != "" {
args = append(args, fmt.Sprintf("--config=%s", c.ConfigPath))
}
args = append(args, fmt.Sprintf("--data-dir=%s", filepath.Join(c.Dir, "data")))

Choose a reason for hiding this comment

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

Could line 66 be merged in line 56 constructor initialization?

killall -9 grafana-server || true
killall -9 tiup-playground || true
killall -9 prometheus || true
killall -9 ng-monitoring-server || true

Choose a reason for hiding this comment

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

Should tikv-cdc be included?

Signed-off-by: pingyu <yuping@pingcap.com>
Copy link

@haojinming haojinming left a comment

Choose a reason for hiding this comment

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

LGTM~

@ti-chi-bot
Copy link
Member

@haojinming: Thanks for your review. The bot only counts approvals 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.

@pingyu pingyu changed the title playground: Supports the TiKV-CDC component playground: Supports TiKV-CDC component Sep 7, 2022
@@ -44,6 +44,7 @@ const (
ComponentDrainer = "drainer"
ComponentPump = "pump"
ComponentCDC = "cdc"
ComponentTiKVCDC = "tikv-cdc"
Copy link
Contributor

@AstroProfundis AstroProfundis Sep 7, 2022

Choose a reason for hiding this comment

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

What's the difference between cdc and tikv-cdc? This is added to the cluster/spec package, so does tiup-cluster need to be aware of the new component as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tikv-cdc is another CDC frame but focus on NoSQL scenario, while cdc focus on SQL database. Please refer to https://github.com/tikv/migration/blob/main/cdc/README.md.

tiup-cluster need to be aware of the new component as well, please see #2022.

outfile=/tmp/tiup-playground-test.out
tiup mirror set $MIRROR
# no tiflash to speed up
tiup-playground $TIDB_VERSION --db 1 --pd 1 --kv 1 --tiflash 0 --kvcdc 1 --kvcdc.version $KVCDC_VERSION > $outfile 2>&1 &
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add this to the current playground integration test? Adding a whole new integration test will add one more Github Actions job and make the CI run time much longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK let me try. I just thought that the separation will make the test easier to be maintained.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not a must, so merging them or not is up to you.

If the new test takes long time to complete, it's better to make it a dedicate one, so that it can run parallel with other jobs, as I can see in the actions run result under this PR, the new job only takes 2~3min to complete, then adding it to the current one could save some time setting up the env in jobs & the time waiting for available runner host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. PTAL, thanks~

Copy link

@zeminzhou zeminzhou left a comment

Choose a reason for hiding this comment

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

LGTM~

@ti-chi-bot
Copy link
Member

@zeminzhou: Thanks for your review. The bot only counts approvals 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.

Signed-off-by: pingyu <yuping@pingcap.com>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 8, 2022
@AstroProfundis AstroProfundis added this to the v1.11.0 milestone Sep 8, 2022
@AstroProfundis
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 6f5d5a7

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 8, 2022
@ti-chi-bot ti-chi-bot merged commit 3671d82 into pingcap:master Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cluster/playground: Support component TiKV-CDC
6 participants