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

s3: increase defaultBufferChanSize #1469

Merged
merged 21 commits into from
Apr 10, 2021
Merged

s3: increase defaultBufferChanSize #1469

merged 21 commits into from
Apr 10, 2021

Conversation

dengqee
Copy link
Contributor

@dengqee dengqee commented Mar 2, 2021

fix bug issue #1259 reported temporarily

What problem does this PR solve?

fix bug issue #1259 reported temporarily

What is changed and how it works?

increase the defaultBufferChanSize of s3, avoiding the l.units[hash].dataChan() full, and blocking the processor.output output rows.
https://github.com/pingcap/ticdc/blob/04e028419387871b80ddc95377751092f03f26ae/cdc/sink/cdclog/utils.go#L164-L170

Check List

Tests

  • No code

Code changes

  • Has persistent data change

Side effects

Related changes

  • Need to cherry-pick to the release branch

Release note

  • s3 sink: increase the defaultBufferChanSize of logSink to avoid blocking

@dengqee
s3: increase defaultBufferChanSize
eb3bdcd 
fix bug issue pingcap#1259 reported temporarily
@amyangfei amyangfei added needs-cherry-pick-release-4.0 Should cherry pick this PR to release-4.0 branch. status/ptal Could you please take a look? labels Mar 2, 2021
@dengqee
Copy link
Contributor Author

dengqee commented Mar 12, 2021

@amyangfei @3pointer @leoppro PTAL

@amyangfei
Copy link
Contributor

amyangfei commented Mar 15, 2021 •
edited
Loading

I think this is a workaround for current sink design. The MySQL sink uses an unlimited buffer (txn cache), but it has some OOM risk. We need a general back pressure mechanism in sink or built on sink. Well I think it is all right to increase the default channel size.

amyangfei
amyangfei reviewed Mar 15, 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.

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 15, 2021
@amyangfei amyangfei added needs-cherry-pick-release-5.0 Should cherry pick this PR to release-5.0 branch. component/pitr Point-in-time recovery component. labels Mar 15, 2021
@overvenus
Copy link
Member

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Mar 19, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Mar 19, 2021
@zier-one
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: eb3bdcd

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

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

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.

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@ti-chi-bot ti-chi-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 24, 2021
@codecov-io
Copy link

codecov-io commented Apr 3, 2021 •
edited
Loading

Codecov Report

Merging #1469 (a0f75f3) into master (c438c60) will increase coverage by 0.0124%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master      #1469        +/-   ##
================================================
+ Coverage   52.9294%   52.9419%   +0.0124%     
================================================
  Files           152        152                
  Lines         16027      16027                
================================================
+ Hits           8483       8485         +2     
+ Misses         6649       6646         -3     
- Partials        895        896         +1

@ti-chi-bot ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Apr 8, 2021
@amyangfei
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 34bde23

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Apr 9, 2021
@amyangfei
Copy link
Contributor

/run-leak-tests

@amyangfei
Copy link
Contributor

/merge

@ti-chi-bot ti-chi-bot merged commit 462870d into pingcap:master Apr 10, 2021
ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Apr 10, 2021
@dengqee @ti-srebot
cherry pick pingcap#1469 to release-4.0
236bd2a 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Apr 10, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #1631

ti-srebot pushed a commit to ti-srebot/ticdc that referenced this pull request Apr 10, 2021
@dengqee @ti-srebot
cherry pick pingcap#1469 to release-5.0
72523a5 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Apr 10, 2021
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #1632

ti-chi-bot pushed a commit that referenced this pull request Apr 12, 2021
@ti-srebot
s3: increase defaultBufferChanSize (#1469) (#1632)
7893add
amyangfei added a commit that referenced this pull request Apr 27, 2021
@ti-srebot @dengqee
@amyangfei @ti-chi-bot
s3: increase defaultBufferChanSize (#1469) #1631
6ea8722 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

Co-authored-by: dengqee <46436429+dengqee@users.noreply.github.com>
Co-authored-by: amyangfei <yangfei@pingcap.com>
Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>
@dengqee dengqee mentioned this pull request Aug 2, 2021
Merged
This was referenced Aug 2, 2021
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amyangfei amyangfei amyangfei left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
component/pitr Point-in-time recovery component. 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. size/XS Denotes a PR that changes 0-9 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?
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@dengqee @amyangfei @overvenus @zier-one @ti-chi-bot @CLAassistant @codecov-io @ti-srebot