-
Notifications
You must be signed in to change notification settings - Fork 314
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
cluster: Support TiKV-CDC component #2022
Conversation
Issue Number: pingcap#1999 Signed-off-by: pingyu <[email protected]>
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Signed-off-by: pingyu <[email protected]>
Codecov ReportBase: 47.67% // Head: 51.46% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2022 +/- ##
==========================================
+ Coverage 47.67% 51.46% +3.79%
==========================================
Files 309 312 +3
Lines 36042 36379 +337
==========================================
+ Hits 17182 18720 +1538
+ Misses 16836 15436 -1400
- Partials 2024 2223 +199
Flags with carried forward coverage won't be shown. Click here to find out 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. |
Signed-off-by: pingyu <[email protected]>
Signed-off-by: pingyu <[email protected]>
Signed-off-by: pingyu <[email protected]>
Signed-off-by: pingyu <[email protected]>
Signed-off-by: pingyu <[email protected]>
@nexustar PTAL, thanks~ |
@pingyu: GitHub didn't allow me to request PR reviews from the following users: haojinming, zeminzhou. Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this: 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 kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM~
@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:
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. |
LGTM~ |
Signed-off-by: pingyu <[email protected]>
@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: 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. |
@@ -28,7 +28,8 @@ func TiDBComponentVersion(comp, version string) string { | |||
ComponentPushwaygate, | |||
ComponentCheckCollector, | |||
ComponentSpark, | |||
ComponentTiSpark: | |||
ComponentTiSpark, | |||
ComponentTiKVCDC: // TiKV-CDC use individual version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any compatibility requirement or the user can always use the latest version of TiKV-CDC regardless of the TiDB version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No requirement. User can always use the latest version by now, and we will maintain the compatibility.
pkg/cluster/spec/spec.go
Outdated
@@ -112,6 +112,7 @@ type ( | |||
Pump map[string]interface{} `yaml:"pump"` | |||
Drainer map[string]interface{} `yaml:"drainer"` | |||
CDC map[string]interface{} `yaml:"cdc"` | |||
TiKVCDC map[string]interface{} `yaml:"tikv-cdc"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use tikv_cdc
to keep the same naming format with other components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason why I chose tikv-cdc
is that:
- I refer to this PR that using
tidb-dashboard
&tidb-dashboard_servers
. - The binary is named as
tikv-cdc
, so usingtikv_cdc
will be error-prone, e.g.run_tikv-cdc.sh.tpl
will has bothtikv-cdc
&tikv_cdc
.
Or how about using kvcdc
? Just as what we add to playground (#2000) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #2017 should also change to use identical name formatting, I'm absolutely sure that mixing -
and _
in the same key name will cause chaos to the end users.
kvcdc
would be a good choice IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay~ I will change to kvcdc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. PTAL~
pkg/cluster/spec/spec.go
Outdated
@@ -127,6 +128,7 @@ type ( | |||
PumpServers []*PumpSpec `yaml:"pump_servers,omitempty"` | |||
Drainers []*DrainerSpec `yaml:"drainer_servers,omitempty"` | |||
CDCServers []*CDCSpec `yaml:"cdc_servers,omitempty"` | |||
TiKVCDCServers []*TiKVCDCSpec `yaml:"tikv-cdc_servers,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, tikv_cdc_servers
native_ssh=true | ||
shift | ||
;; | ||
--staging) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This staging mirror is not used anywhere in this PR
version="nightly" | ||
topo_name="tikv_cdc" | ||
test_tls=false | ||
mirror="https://tiup-mirrors.pingcap.com/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiup mirror set --reset
will do the same as tiup mirror set https://tiup-mirrors.pingcap.com
done | ||
|
||
# set mirror | ||
tiup mirror set $mirror |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as the staging mirror is not used, i think it's not necessary to build tiup
and use it to manually set the mirror.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiup
is also used for installing tikv-cdc
component for test case of tiup cluster patch
. I remove the tiup mirror
here, but keep the tiup
target & function.
Imported bool `yaml:"imported,omitempty"` | ||
Patched bool `yaml:"patched,omitempty"` | ||
IgnoreExporter bool `yaml:"ignore_exporter,omitempty"` | ||
Port int `yaml:"port" default:"8600"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The port should also be added to portTypes
list in pkg/cluster/spec/validate.go
at around L619, other wise the port conflict check is not working for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that the Port
is already in the list for portTypes
where portConflictsDetect
get port value from. So the port conflict check should work.
And I add a test case to verify port conflict check in integrated test for tikv-cdc. PTAL~
Signed-off-by: pingyu <[email protected]>
Signed-off-by: pingyu <[email protected]>
Signed-off-by: pingyu <[email protected]>
Signed-off-by: pingyu <[email protected]>
Signed-off-by: pingyu <[email protected]>
@AstroProfundis Thanks for your review ! All comments are addressed or replied, PTAL~ |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: c1e81fd
|
@pingyu: Your PR was out of date, I have automatically updated it for you. 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 <[email protected]>
Signed-off-by: pingyu <[email protected]>
Added cluster version check. PTAL, thanks~ |
/merge |
This pull request has been accepted and is ready to merge. Commit hash: 5304c07
|
Issue Number: #1999
Signed-off-by: pingyu [email protected]
What problem does this PR solve?
#1999
What is changed and how it works?
TiUP cluster supports the TiKV-CDC component.
Check List
Tests
Code changes
Side effects
Related changes
Release notes: