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

pipeline(ticdc): split dataflow in sinkNode #5943

Merged
merged 5 commits into from
Jun 23, 2022
Merged

pipeline(ticdc): split dataflow in sinkNode #5943

merged 5 commits into from
Jun 23, 2022

Conversation

CharlesCheung96
Copy link
Contributor

@CharlesCheung96 CharlesCheung96 commented Jun 20, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: close #5942

What is changed and how it works?

Split dataflow (emit & flush Redolog) in sinkNode instead of tableSink：

  1. Decouple Redolog data stream from tableSink to facilitate sink module refactoring.
    Ref: sinkv2(ticdc): add interfaces #5929
  2. Manage the Redolog data stream in sinkNode to facilitate redoManager refactoring.
    Ref: redo(ticdc): make implementation in manager asynchronously #5683

Check List

Tests

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

Questions

Will it cause performance regression or break compatibility?

No

Do you need to update user documentation, design documentation or monitoring documentation?

No

Release note 

`None`.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jun 20, 2022 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • hi-rustin
  • zhaoxinyu

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-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 20, 2022
@ti-chi-bot
Copy link
Member

@CharlesCheung96: The label(s) area/cdc cannot be applied, because the repository doesn't have them.

In response to this:

/area cdc

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.

1 similar comment
@ti-chi-bot
Copy link
Member

@CharlesCheung96: The label(s) area/cdc cannot be applied, because the repository doesn't have them.

In response to this:

/area cdc

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.

@ti-chi-bot ti-chi-bot added the area/ticdc Issues or PRs related to TiCDC. label Jun 20, 2022
@CharlesCheung96 CharlesCheung96 force-pushed the split_dataflow_in_sinkNode branch from 67b69eb to bde4c16 Compare June 20, 2022 14:53
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2022 •
edited
Loading

Codecov Report

Merging #5943 (bde4c16) into master (6a451ea) will increase coverage by 0.2276%.
The diff coverage is 61.8887%.

Flag Coverage Δ
cdc 62.4525% <50.6976%> (+0.4725%) ⬆️
dm 51.9821% <23.2142%> (+0.0519%) ⬆️
engine 62.4733% <76.0869%> (+0.2887%) ⬆️

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

@@               Coverage Diff                @@
##             master      #5943        +/-   ##
================================================
+ Coverage   57.0764%   57.3041%   +0.2276%     
================================================
  Files           682        678         -4     
  Lines         80224      79989       -235     
================================================
+ Hits          45789      45837        +48     
+ Misses        30146      29912       -234     
+ Partials       4289       4240        -49

@CharlesCheung96 CharlesCheung96 force-pushed the split_dataflow_in_sinkNode branch from 1399734 to 3560a58 Compare June 22, 2022 02:52
@CharlesCheung96 CharlesCheung96 force-pushed the split_dataflow_in_sinkNode branch from 3560a58 to 73e4216 Compare June 22, 2022 02:54
@CharlesCheung96 CharlesCheung96 marked this pull request as ready for review June 22, 2022 02:54
@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 Jun 22, 2022
@CharlesCheung96
Copy link
Contributor Author

/run-all-tests

@ti-chi-bot ti-chi-bot requested a review from amyangfei June 22, 2022 16:43
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 23, 2022
Rustin170506
Rustin170506 reviewed Jun 23, 2022
View reviewed changes
cdc/processor/pipeline/sink.go Outdated
if resolved.Ts > n.targetTs {
resolved = model.NewResolvedTs(n.targetTs)
}

if n.redoManager != nil && n.redoManager.Enabled() {
// redo log do not support batch resolve mode
Copy link
Member

Choose a reason for hiding this comment

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

I think we could have explain it in more detail.

cdc/processor/pipeline/sink.go Show resolved Hide resolved
Rustin170506
Rustin170506 approved these changes Jun 23, 2022
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.

#5943 (comment)

rest LGTM

/hold

Please feel free to unhold this when you add the comments.

@ti-chi-bot ti-chi-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 23, 2022
@Rustin170506
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: be2ed0f

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 23, 2022
@Rustin170506
Copy link
Member

/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 Jun 23, 2022
@ti-chi-bot ti-chi-bot merged commit 0294e28 into pingcap:master Jun 23, 2022
CharlesCheung96 added a commit to ti-chi-bot/tiflow that referenced this pull request Jul 6, 2022
@CharlesCheung96
pipeline(ticdc): split dataflow in sinkNode (pingcap#5943)
e543bdd 
close pingcap#5942
CharlesCheung96 added a commit to ti-chi-bot/tiflow that referenced this pull request Jul 7, 2022
@CharlesCheung96
pipeline(ticdc): split dataflow in sinkNode (pingcap#5943)
32bed6c 
close pingcap#5942
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

@zhaoxinyu zhaoxinyu zhaoxinyu approved these changes

@amyangfei amyangfei Awaiting requested review from amyangfei

Assignees
No one assigned
Labels
area/ticdc Issues or PRs related to TiCDC. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 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.

split dataflow in sinkNode
5 participants
@CharlesCheung96 @ti-chi-bot @codecov-commenter @Rustin170506 @zhaoxinyu