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.

Sign up for GitHub

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

Jump to bottom

cdc, cmd: allow customized changefeed id #727

Merged
merged 7 commits into from
Jul 13, 2020
Merged

cdc, cmd: allow customized changefeed id #727

merged 7 commits into from
Jul 13, 2020

Conversation

overvenus
Copy link
Member

What problem does this PR solve?

Allow customized changefeed id.

What is changed and how it works?

Create changefeed with an argurment --changefeed for -c to set its name.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has exported function/method change
  • Has persistent data change

Side effects

  • Increased code complexity

Related changes

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

Release note

  • Support customize changefeed name.

@overvenus
cdc, cmd: allow customized changefeed id
9d38815 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus overvenus added the subject/usability Denotes an issue or pull request is related to usability. label Jul 7, 2020
@overvenus overvenus added this to the v4.0.3 milestone Jul 7, 2020
@overvenus
Merge branch 'master' into cf-name
683af8e 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@overvenus
Copy link
Member Author

/run-integration-tests

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2020 •
edited
Loading

Codecov Report

Merging #727 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #727   +/-   ##
===========================================
  Coverage   34.6297%   34.6297%           
===========================================
  Files            91         91           
  Lines          9899       9899           
===========================================
  Hits           3428       3428           
  Misses         6159       6159           
  Partials        312        312

overvenus
overvenus commented Jul 8, 2020
View reviewed changes
cdc/model/changefeed.go Outdated Show resolved Hide resolved
amyangfei
amyangfei reviewed Jul 10, 2020
View reviewed changes
cdc/kv/etcd.go Show resolved Hide resolved
cdc/model/errors.go Outdated Show resolved Hide resolved
@overvenus
address comments
209c3ad 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@amyangfei amyangfei added the status/ptal Could you please take a look? label Jul 10, 2020
@overvenus
Copy link
Member Author

PTAL

amyangfei
amyangfei approved these changes Jul 10, 2020
View reviewed changes
Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

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

LGTM

@amyangfei amyangfei added LGT1 and removed status/ptal Could you please take a look? labels Jul 10, 2020
zier-one
zier-one approved these changes Jul 13, 2020
View reviewed changes
Copy link
Contributor

@zier-one zier-one 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-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 13, 2020
@ti-srebot
Copy link
Contributor

@leoppro,Thanks for your review.

@zier-one zier-one removed the LGT1 label Jul 13, 2020
@zier-one
Copy link
Contributor

/lgtm

@overvenus
Copy link
Member Author

/merge

@ti-srebot
Copy link
Contributor

Sorry @overvenus, you don't have permission to trigger auto merge event on this branch. The number of LGTM for this PR is 1 while it needs 2 at least

@overvenus
Copy link
Member Author

/run-integration-tests

@zier-one
Copy link
Contributor

@amyangfei could you comment 'LGTM' again to trigger the merge robot, please?

@amyangfei
Copy link
Contributor

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 13, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 13, 2020
@ti-srebot
Copy link
Contributor

@amyangfei,Thanks for your review.

@zier-one
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 13, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@overvenus merge failed.

@zier-one
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 737

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@overvenus merge failed.

@amyangfei
Copy link
Contributor

/merge

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@overvenus merge failed.

@zier-one
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@overvenus merge failed.

@zier-one
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 2ffc0e7 into pingcap:master Jul 13, 2020
@overvenus overvenus deleted the cf-name branch July 14, 2020 03:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amyangfei amyangfei amyangfei approved these changes

@zier-one zier-one zier-one approved these changes

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. subject/usability Denotes an issue or pull request is related to usability.
Projects
None yet
Milestone
v4.0.3
Development

Successfully merging this pull request may close these issues.
6 participants
@overvenus @codecov-commenter @ti-srebot @zier-one @amyangfei @CLAassistant