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

cli(refactor): add create changefeed cmd #2539

Merged

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Aug 16, 2021

What problem does this PR solve?

split from https://github.com/pingcap/ticdc/pull/2482

What is changed and how it works?

add create changefeed cmd.

Check List

Tests

None

Code changes

  • Has exported function/method change
  • Has exported variable/fields change

Side effects

None

Related changes

None

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 16, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • asddongmen
  • overvenus

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 16, 2021
@ti-chi-bot ti-chi-bot requested review from liuzix and lonng August 16, 2021 07:12
@ti-chi-bot ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 16, 2021
@Rustin170506
Copy link
Member Author

/uncc liuzx lonng

@ti-chi-bot ti-chi-bot removed request for lonng and liuzix August 16, 2021 07:12
@Rustin170506 Rustin170506 changed the title wip: cli(refactor): add create changefeed cmd cli(refactor): add create changefeed cmd Aug 17, 2021
@ti-chi-bot 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 Aug 17, 2021
@Rustin170506
Copy link
Member Author

Rustin170506 commented Aug 17, 2021

/cc @amyangfei @overvenus @3AceShowHand

Could you please take a look?

Also, I don't know if I should decouple the getInfo method, because it actually does two things: 1. gets and completes the cfg 2. generates info.

But since we use cfg in info, we put it together. Do you think it needs to be decoupled? Also in the process of completing the cfg we are constantly outputting warning messages to cmd. I'm not sure if decoupling will have any effect on these warnings. For example, the order and timing of its appearance. Can you guys help with some advice?

@overvenus
Copy link
Member

Also in the process of completing the cfg we are constantly outputting warning messages to cmd.

What are these warning messages?

@Rustin170506
Copy link
Member Author

What are these warning messages?

We output a lot of warnings in the process of completing the cfg.

@Rustin170506 Rustin170506 added the needs-cherry-pick-release-5.2 Should cherry pick this PR to release-5.2 branch. label Aug 19, 2021
@asddongmen
Copy link
Contributor

rest LGTM

@Rustin170506
Copy link
Member Author

@amyangfei @overvenus @asddongmen This PR tested in pingcap/ticdc@f13b168. All tests passed.

The code changes are quite significant and may require very careful review. Thanks! ❤️

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 20, 2021
@Rustin170506
Copy link
Member Author

/run-all-tests

@Rustin170506
Copy link
Member Author

/run-all-tests

@Rustin170506
Copy link
Member Author

@amyangfei @overvenus ping~

Could you please take a look?

Copy link
Member

@overvenus overvenus left a comment

Choose a reason for hiding this comment

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

👍

@ti-chi-bot 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 Aug 23, 2021
@overvenus
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 88f51c1

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 23, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #2539 (5e98baa) into master (f3fdd65) will increase coverage by 5.0935%.
The diff coverage is 68.9024%.

@@               Coverage Diff                @@
##             master      #2539        +/-   ##
================================================
+ Coverage   56.0786%   61.1722%   +5.0935%     
================================================
  Files           169        161         -8     
  Lines         20605      17915      -2690     
================================================
- Hits          11555      10959       -596     
+ Misses         7919       5958      -1961     
+ Partials       1131        998       -133     

@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #2611.

@Rustin170506 Rustin170506 deleted the rustin-patch-changefeed-create branch August 15, 2022 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cherry-pick-release-5.2 Should cherry pick this PR to release-5.2 branch. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants