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

sink: remove cdclog. #3599

Merged
merged 6 commits into from
Dec 1, 2021
Merged

sink: remove cdclog. #3599

merged 6 commits into from
Dec 1, 2021

Conversation

3AceShowHand
Copy link
Contributor

@3AceShowHand 3AceShowHand commented Nov 23, 2021 •
edited by Rustin170506
Loading

What problem does this PR solve?

This PR remove all cdclog related code, include S3Sink and fileSink.

What is changed and how it works?

  • remove all related code and test cases.

Check List

Tests

  • No code

Code changes

Side effects

Related changes

Release note 

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 23, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • hi-rustin
  • 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 release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 23, 2021
Rustin170506
Rustin170506 reviewed Nov 24, 2021
View reviewed changes
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

The code change looks good.

  1. please fill in the description of pr
  2. please remove the corresponding integration test

@3AceShowHand
Copy link
Contributor Author

/run-all-tests

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2021
overvenus
overvenus reviewed Nov 30, 2021
View reviewed changes
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.

After addressing https://github.com/pingcap/ticdc/pull/3599#pullrequestreview-814356317, LGTM

@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 1, 2021
@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 1, 2021
@3AceShowHand
Copy link
Contributor Author

/run-all-tests

@codecov-commenter
Copy link

codecov-commenter commented Dec 1, 2021 •
edited
Loading

Codecov Report

Merging #3599 (53caaa5) into master (1a4a20e) will increase coverage by 0.1719%.
The diff coverage is n/a.

Flag Coverage Δ
cdc 57.6470% <ø> (+0.3131%) ⬆️
dm 56.4371% <ø> (+0.0504%) ⬆️

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

@@               Coverage Diff                @@
##             master      #3599        +/-   ##
================================================
+ Coverage   56.8249%   56.9969%   +0.1719%     
================================================
  Files           456        456                
  Lines         54147      54138         -9     
================================================
+ Hits          30769      30857        +88     
+ Misses        20183      20073       -110     
- Partials       3195       3208        +13

@3AceShowHand
Copy link
Contributor Author

/run-all-tests

overvenus
overvenus approved these changes Dec 1, 2021
View reviewed changes
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.

LGTM

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

image

The description here also needs to be handled. Let me change it for you.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 1, 2021
@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 Dec 1, 2021
@zier-one
Copy link
Contributor

zier-one commented Dec 1, 2021

LGTM from PM. We can withdraw the cdclog sink.

@amyangfei
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 53caaa5

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 1, 2021
@ti-chi-bot ti-chi-bot merged commit 7acb929 into pingcap:master Dec 1, 2021
@3pointer 3pointer mentioned this pull request Dec 6, 2021
Merged
1 task
okJiang pushed a commit to okJiang/tiflow that referenced this pull request Dec 8, 2021
@3AceShowHand @okJiang
sink: remove cdclog. (pingcap#3599)
7097267
@amyangfei amyangfei mentioned this pull request Dec 8, 2021
Closed
@dengqee
Copy link
Contributor

dengqee commented Dec 8, 2021

Can I ask why you removed cdclog sink?

@amyangfei
Copy link
Contributor

This feature will not be maintained in the latest version, would you please give more details from the view of product @leoppro ?

@joccau joccau mentioned this pull request Dec 9, 2021
Merged
4 tasks
@joccau joccau mentioned this pull request Dec 24, 2021
Merged
4 tasks
@dveeden dveeden mentioned this pull request Jan 3, 2022
Closed
12 tasks
@dveeden dveeden mentioned this pull request Feb 3, 2022
Merged
14 tasks
@coderplay
Copy link

The same question here. Why you guys removed it?

@gegao-db gegao-db mentioned this pull request May 17, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rustin170506 Rustin170506 Rustin170506 approved these changes

@overvenus overvenus overvenus approved these changes

@amyangfei amyangfei Awaiting requested review from amyangfei

@asddongmen asddongmen Awaiting requested review from asddongmen

@zier-one zier-one Awaiting requested review from zier-one

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@3AceShowHand @ti-chi-bot @codecov-commenter @Rustin170506 @zier-one @amyangfei @dengqee @coderplay @overvenus