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(ticdc): add flush worker to flush row changed events #4633

Merged
merged 16 commits into from
Feb 24, 2022
Merged

sink(ticdc): add flush worker to flush row changed events #4633

merged 16 commits into from
Feb 24, 2022

Conversation

Rustin170506
Copy link
Member

@Rustin170506 Rustin170506 commented Feb 20, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: ref #4423

What is changed and how it works?

add flush worker to flush row changed events.

Main processes:

  1. Gather messages (multiple partition messages are collected)
  2. Classified by parition
  3. The events are encoded and sent to the producer by partition.

Check List

Tests

  • Unit test
  • benchmark

Code changes

None

Side effects

None

Related changes

None

Release note 

None

@ti-chi-bot
Copy link
Member

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

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • asddongmen
  • 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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 20, 2022
@Rustin170506
Copy link
Member Author

/hold

base on #4630

@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 Feb 20, 2022
@Rustin170506 Rustin170506 marked this pull request as draft February 20, 2022 08:20
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 20, 2022 •
edited
Loading

Codecov Report

Merging #4633 (29cfb10) into master (9607554) will decrease coverage by 0.4803%.
The diff coverage is 39.2650%.

Flag Coverage Δ
cdc 59.7705% <44.4141%> (-0.1517%) ⬇️
dm 51.3003% <32.2139%> (-0.7285%) ⬇️

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

@@               Coverage Diff                @@
##             master      #4633        +/-   ##
================================================
- Coverage   55.6402%   55.1598%   -0.4804%     
================================================
  Files           494        514        +20     
  Lines         61283      63713      +2430     
================================================
+ Hits          34098      35144      +1046     
- Misses        23750      25079      +1329     
- Partials       3435       3490        +55

@Rustin170506 Rustin170506 changed the title ticdc(sink): add flush worker to flush row changed events sink(ticdc): add flush worker to flush row changed events Feb 20, 2022
@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 Feb 22, 2022
@Rustin170506
Copy link
Member Author

/run-kafka-integration-test /tidb=pr/32434

@ti-chi-bot ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 23, 2022
@Rustin170506 Rustin170506 marked this pull request as ready for review February 23, 2022 02:47
@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 Feb 23, 2022
@Rustin170506
Copy link
Member Author

/run-kafka-integration-test /tidb=pr/32434

/run-integration-test /tidb=pr/32434

@Rustin170506 Rustin170506 added area/ticdc Issues or PRs related to TiCDC. component/sink Sink component. labels Feb 23, 2022
@Rustin170506
Copy link
Member Author

Rustin170506 commented Feb 23, 2022 •
edited
Loading

/uncc asddongmen
/cc @overvenus

Because asddongmen on leave, so please help to take a look.

@ti-chi-bot ti-chi-bot requested review from overvenus and removed request for asddongmen February 23, 2022 13:28
@Rustin170506
Copy link
Member Author

I'm sorry I tested with my own computer, but currently the runc runtime on tispace is not working properly due to some problems. idc's machine is heavily oversold. So maybe my local machine is an acceptable option.

Let's do a more realistic performance test before GA, but this current result should also be informative.

@Rustin170506
Copy link
Member Author

/run-all-tests

@Rustin170506
Copy link
Member Author

/run-verify

overvenus
overvenus reviewed Feb 24, 2022
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

cdc/sink/mq_flush_worker.go Outdated Show resolved Hide resolved
cdc/sink/mq_flush_worker.go Outdated Show resolved Hide resolved
@Rustin170506
Copy link
Member Author

/run-all-tests

@zhaoxinyu
Copy link
Contributor

/LGTM

@Rustin170506
Copy link
Member Author

/LGTM

@zhaoxinyu You need to use GitHub approve feature :D

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

/run-all-tests

@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 Feb 24, 2022
@Rustin170506
Copy link
Member Author

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 899c959

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

/run-verify

@ti-chi-bot ti-chi-bot merged commit ed2ac96 into pingcap:master Feb 24, 2022
@Rustin170506 Rustin170506 deleted the rustin-patch-sink-flush-worker branch February 24, 2022 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhaoxinyu zhaoxinyu zhaoxinyu approved these changes

@asddongmen asddongmen asddongmen approved these changes

@overvenus overvenus Awaiting requested review from overvenus

Assignees
No one assigned
Labels
area/ticdc Issues or PRs related to TiCDC. component/sink Sink component. 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.
6 participants
@Rustin170506 @ti-chi-bot @codecov-commenter @zhaoxinyu @overvenus @asddongmen