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

config: remove per-table-memory-quota min value check #3022

Merged
merged 5 commits into from
Oct 14, 2021
Merged

config: remove per-table-memory-quota min value check #3022

merged 5 commits into from
Oct 14, 2021

Conversation

overvenus
Copy link
Member

@overvenus overvenus commented Oct 12, 2021 •
edited
Loading

What problem does this PR solve?

Remove per-table-memory-quota min value check, because it is
possible to capture thousands of tables, and in this case, 6MB is
too large.

Also, it changes the default quota from 20MB to 10MB. Tests show
that no obvious bottleneck for 10MB.

Cc https://github.com/pingcap/ticdc/pull/2561

Metrics: https://gist.github.com/overvenus/183013d6b68c73f6b31d0dc1e5fb2bec

Quota = 1MB vs Quota = 10MB vs Quota = 20MB
image
image
image 4.5s vs 49s vs 58s
image 41k vs 361k vs 474k
image

Check List

Tests

  • Manual test (add detailed scripts or steps below)

See above.

Related changes

  • Need to cherry-pick to the release branch

Release note 

None

@overvenus overvenus added component/configuration Configuration 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. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. needs-cherry-pick-release-5.2 Should cherry pick this PR to release-5.2 branch. labels Oct 12, 2021
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 12, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • amyangfei
  • liuzix

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 Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 12, 2021
@overvenus
config: remove per-table-memory-quota min value check
eecd4c9 
Remove per-table-memory-quota min value check, because it is
possible to capture thousands of tables, and in this case, 6MB is
too large.

Also, it changes the default quota from 20MB to 10MB. Tests show
that no obvious bottleneck for 10MB.

Signed-off-by: Neil Shen <overvenus@gmail.com>
@amyangfei amyangfei added this to the v5.3.0 milestone Oct 12, 2021
amyangfei
amyangfei reviewed Oct 12, 2021
View reviewed changes
pkg/config/config_test.go
@@ -117,5 +117,5 @@ func TestServerConfigValidateAndAdjust(t *testing.T) {
require.Regexp(t, ".*does not contain a port", conf.ValidateAndAdjust())
conf.AdvertiseAddr = "advertise:1234"
conf.PerTableMemoryQuota = 1
Copy link
Contributor

@amyangfei amyangfei Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Can add a test, when conf.PerTableMemoryQuota is zero, ValidateAndAdjust will reset it to default quota

@overvenus
address comment
9982e2a 
Signed-off-by: Neil Shen <overvenus@gmail.com>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 13, 2021
@liuzix
Copy link
Contributor

liuzix commented Oct 13, 2021

We should probably discuss which branches this PR needs to be cherry-picked to.

@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 Oct 13, 2021
@overvenus
Copy link
Member Author

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 9982e2a

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

/run-integration-tests

@overvenus
Copy link
Member Author

/run-kafka-tests
/run-integration-tests

1 similar comment
@overvenus
Copy link
Member Author

/run-kafka-tests
/run-integration-tests

@ti-chi-bot
Copy link
Member

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

At the same time I will also trigger all tests for you:

/run-all-tests

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.

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.

@amyangfei
Copy link
Contributor

/run-kafka-tests

@ti-chi-bot ti-chi-bot merged commit be1f3fe into pingcap:master Oct 14, 2021
ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Oct 14, 2021
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#3022
4395f19 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Oct 14, 2021
Merged
@ti-chi-bot
Copy link
Member

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

@ti-chi-bot ti-chi-bot mentioned this pull request Oct 14, 2021
Merged
@ti-chi-bot
Copy link
Member

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

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Oct 14, 2021
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#3022
2cfb87d 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Oct 14, 2021
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#3022
fd1074b 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Oct 14, 2021
Merged
@ti-chi-bot
Copy link
Member

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

ti-chi-bot pushed a commit to ti-chi-bot/tiflow that referenced this pull request Oct 14, 2021
@overvenus @ti-chi-bot
This is an automated cherry-pick of pingcap#3022
de064bb 
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot ti-chi-bot mentioned this pull request Oct 14, 2021
Merged
@ti-chi-bot
Copy link
Member

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

ti-chi-bot added a commit that referenced this pull request Oct 14, 2021
@ti-chi-bot
config: remove per-table-memory-quota min value check (#3022) (#3038)
4c01bc6
ti-chi-bot added a commit that referenced this pull request Oct 14, 2021
@ti-chi-bot
config: remove per-table-memory-quota min value check (#3022) (#3037)
36c8503
ti-chi-bot added a commit that referenced this pull request Oct 14, 2021
@ti-chi-bot
config: remove per-table-memory-quota min value check (#3022) (#3040)
175ca01
ti-chi-bot added a commit that referenced this pull request Oct 14, 2021
@ti-chi-bot
config: remove per-table-memory-quota min value check (#3022) (#3039)
8732b1e
@overvenus overvenus mentioned this pull request Oct 27, 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

@liuzix liuzix liuzix approved these changes

Assignees
No one assigned
Labels
component/configuration Configuration 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. needs-cherry-pick-release-5.1 Should cherry pick this PR to release-5.1 branch. needs-cherry-pick-release-5.2 Should cherry pick this PR to release-5.2 branch. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 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
v5.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@overvenus @ti-chi-bot @liuzix @amyangfei