Skip to content

cdc: Lightning physical import ignore by cdc #17895

Merged
ti-chi-bot[bot] merged 8 commits intotikv:masterfrom
3AceShowHand:lightning-cdc-skip-row
Dec 3, 2024
Merged

cdc: Lightning physical import ignore by cdc #17895
ti-chi-bot[bot] merged 8 commits intotikv:masterfrom
3AceShowHand:lightning-cdc-skip-row

Conversation

@3AceShowHand
Copy link
Contributor

@3AceShowHand 3AceShowHand commented Nov 26, 2024 •
edited
Loading

What is changed and how it works?

Issue Number: Close #17897

What's Changed:

* TiKV-CDC skip the row if the 17th bit of the txn_source is set, which indicate it comes from the Lightning physical import
* sst-service initialize the txn_sst_writer also set the txn_source

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

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

The blow images shows that:

  1. No row change event data send to the TiCDC
  2. TiKV-CDC still triggers incremental scan, this is comes from the Lightning physical imported data
image image image image image

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Release note

None

@3AceShowHand
cdc filter data by the tag
3d9c9ac 
Signed-off-by: 3AceShowHand <jinl1037@hotmail.com>
@3AceShowHand
sst service set txn source for the writer
7d465c6 
Signed-off-by: 3AceShowHand <jinl1037@hotmail.com>
@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-linked-issue dco-signoff: yes Indicates the PR's author has signed the dco. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 26, 2024
@3AceShowHand
fix the make clippy
167452a 
Signed-off-by: 3AceShowHand <jinl1037@hotmail.com>
@3AceShowHand
Copy link
Contributor Author

3AceShowHand commented Nov 26, 2024

/retest

@3AceShowHand
add some unit test
e40e097 
Signed-off-by: 3AceShowHand <jinl1037@hotmail.com>
@ti-chi-bot ti-chi-bot 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 Nov 26, 2024
@3AceShowHand
fix the make format
96235bf 
Signed-off-by: 3AceShowHand <jinl1037@hotmail.com>
lance6716
lance6716 approved these changes Nov 27, 2024
View reviewed changes
Copy link
Contributor

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

sst_importer part LGTM

components/sst_importer/src/sst_importer.rs
}

pub fn new_raw_writer(&self, db: &E, mut meta: SstMeta) -> Result<RawSstWriter<E>> {
pub fn new_raw_writer(&self, db: &E, mut meta: SstMeta, _: u64) -> Result<RawSstWriter<E>> {
Copy link
Contributor

@lance6716 lance6716 Nov 27, 2024

Choose a reason for hiding this comment

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

I'm not sure this is a good chance to let BR restore also be compatible with CDC

Copy link
Contributor Author

@3AceShowHand 3AceShowHand Nov 27, 2024

Choose a reason for hiding this comment

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

what is the usage of the raw_sst_writer ? it looks for the raw kv functionality ?

Copy link
Contributor

@lance6716 lance6716 Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Seems no one is using it. I wrongly think it's used by BR 😂 Maybe ask the code author of this line

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Nov 27, 2024

@lance6716: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

sst_importer part LGTM

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-sigs/prow repository.

hicqu
hicqu reviewed Nov 29, 2024
View reviewed changes
hicqu
hicqu reviewed Nov 29, 2024
View reviewed changes
@3AceShowHand
merge conditions together
f0e5815 
Signed-off-by: 3AceShowHand <jinl1037@hotmail.com>
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 29, 2024
@3AceShowHand
Copy link
Contributor Author

3AceShowHand commented Dec 2, 2024

/retest

overvenus
overvenus reviewed Dec 2, 2024
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.

Rest LGTM

@3AceShowHand @overvenus
Update components/cdc/src/txn_source.rs
360b270 
Co-authored-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Ling Jin <7138436+3AceShowHand@users.noreply.github.com>
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Dec 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hicqu, lance6716, overvenus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 3, 2024
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Dec 3, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-11-29 09:11:28.168335592 +0000 UTC m=+800475.787990108: ☑️ agreed by hicqu.
  • 2024-12-03 03:20:44.937868695 +0000 UTC m=+1125032.557523205: ☑️ agreed by overvenus.

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Dec 3, 2024
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Dec 3, 2024

@3AceShowHand: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Details

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.

@3AceShowHand
Copy link
Contributor Author

3AceShowHand commented Dec 3, 2024

/retest

@ti-chi-bot ti-chi-bot bot merged commit 81941fc into tikv:master Dec 3, 2024
@ti-chi-bot ti-chi-bot bot added this to the Pool milestone Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@overvenus overvenus overvenus approved these changes

@hicqu hicqu hicqu approved these changes

@lance6716 lance6716 Awaiting requested review from lance6716

Assignees

No one assigned

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

Pool

Development

Successfully merging this pull request may close these issues.

TiKV-CDC skip the data imported by the Lightning physical mode

4 participants

@3AceShowHand @lance6716 @overvenus @hicqu