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

puller,mounter,processor: always pull the old value internally #2271

Merged
merged 23 commits into from
Jul 16, 2021
Merged

puller,mounter,processor: always pull the old value internally #2271

merged 23 commits into from
Jul 16, 2021

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Jul 13, 2021 •
edited
Loading

What problem does this PR solve?

ref: #2301

What is changed and how it works?

ticdc always pull the old value internally.

Because our new collation feature needs the old value to work properly. However, some users will not use old value, but may use new collation, so it needs to be configured by the user. (Users don't even know if they have new collation on)

To simplify this configuration and to make it easier for the user, we decided to:

  1. always use old value internally.
  2. the data output to the user needs to be in the correct format (without the old value configuration on, the old value cannot be output)

Check List

Tests

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

Code changes

None

Side effects

  • Possible performance regression

Related changes

  • Need to cherry-pick to the release branch

Release note

  • puller,mounter,processor: always pull the old value internally

@ti-chi-bot ti-chi-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 13, 2021
@Rustin170506
Copy link
Member Author

Rustin170506 commented Jul 13, 2021 •
edited
Loading

ref: #2268

/hold

  • needs comments
  • More testing needed

@ti-chi-bot ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2021
@Rustin170506
Copy link
Member Author

/cc @overvenus

cc: @SuperSupeng

amyangfei
amyangfei reviewed Jul 13, 2021
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.

After TiCDC always pulls kv events with old-value enabled, unmarshalIndexKVEntry will not be used anymore, we can add some comment about it.

cdc/processor/pipeline/puller.go Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 13, 2021
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 13, 2021
@Rustin170506
Copy link
Member Author

Rustin170506 commented Jul 13, 2021 •
edited
Loading

After TiCDC always pulls kv events with old-value enabled, unmarshalIndexKVEntry will not be used anymore, we can add some comment about it.

Thanks for your review! Unused code deleted. All the tests passed.

@Rustin170506
Copy link
Member Author

/unhold

@ti-chi-bot ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2021
@amyangfei
Copy link
Contributor

amyangfei commented Jul 14, 2021 •
edited
Loading

This PR needs to cherry-pick to all release branches, right? @hi-rustin

@amyangfei amyangfei added the subject/usability Denotes an issue or pull request is related to usability. label Jul 14, 2021
@Rustin170506 Rustin170506 added needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. labels Jul 14, 2021
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • amyangfei
  • 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 status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jul 16, 2021
@Rustin170506
Copy link
Member Author

/run-all-tests

/assign @amyangfei

Could you help me trigger the merge process?

@amyangfei
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 6219bc1

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 16, 2021
@Rustin170506
Copy link
Member Author

/run-unit-tests

@ti-chi-bot ti-chi-bot merged commit 78987aa into pingcap:master Jul 16, 2021
ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Jul 16, 2021
@Rustin170506 @ti-chi-bot
This is an automated cherry-pick of pingcap#2271
700110b 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Jul 16, 2021
Merged
@ti-chi-bot
Copy link
Member

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

@ti-chi-bot
Copy link
Member

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

This was referenced Jul 16, 2021
Merged
Merged
@ti-chi-bot
Copy link
Member

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

@Rustin170506 Rustin170506 deleted the rustin-patch-puller branch July 16, 2021 11:44
@Rustin170506 Rustin170506 mentioned this pull request Jul 16, 2021
Merged
Rustin170506 added a commit to ti-chi-bot/tiflow that referenced this pull request Jul 19, 2021
@Rustin170506
puller,mounter,processor: always pull the old value internally (pingc…
e7ac631 
…ap#2271)
This was referenced Jul 19, 2021
Merged
Merged
Merged
ti-chi-bot added a commit that referenced this pull request Jul 19, 2021
@ti-chi-bot
puller,mounter,processor: always pull the old value internally (#2271) (
7c2999e 
#2306)
ti-chi-bot added a commit that referenced this pull request Jul 28, 2021
@ti-chi-bot
puller,mounter,processor: always pull the old value internally (#2271) (
b09085f 
#2304)
ti-chi-bot added a commit that referenced this pull request Jul 30, 2021
@ti-chi-bot
puller,mounter,processor: always pull the old value internally (#2271) (
7bba9f7 
#2305)
@Rustin170506 Rustin170506 mentioned this pull request Aug 25, 2021
Closed
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

@overvenus overvenus overvenus approved these changes

@liuzix liuzix Awaiting requested review from liuzix

@ben1009 ben1009 Awaiting requested review from ben1009

Assignees

@amyangfei amyangfei

Labels
needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. 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. status/ptal Could you please take a look? subject/usability Denotes an issue or pull request is related to usability.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Rustin170506 @amyangfei @codecov-commenter @ti-chi-bot @ben1009 @overvenus