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.

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

[3.5] Backport DefaultSnapshotCount 10K #18589

Open
wants to merge 1 commit into
base: release-3.5
Choose a base branch
from

Conversation

clement2026
Copy link
Contributor

An experiment in #17098 (comment) suggests that setting a low snapshot-count can reduce the heap size. 

v3.5 might benefit from setting DefaultSnapshotCount to 10_000, which is already the default in v3.6.


Here is a table copied from #17098 (comment), showing how heap size is affected by --snapshot-count and --experimental-snapshot-catchup-entries.

  • putSize: average size of put requests
putSize --snapshot-count --experimental-snapshot
-catchup-entries
heap size
v3.5.16
heap size
v3.6.0-alpha.0
1 KB 10000 5000 6 MB ~ 28 MB 13 MB ~ 31.7 MB
10 KB 10000 5000 64 MB ~ 180 MB
100 KB 10000 5000 569 MB ~ 1.5 GB 536 MB ~ 1.62 GB
1 MB 10000 5000 5 GB ~ 14.2 GB
--- --- --- --- ---
1 KB 100000 5000 15 MB ~ 143 MB 15 MB ~ 152 MB
10 KB 100000 5000 67 MB ~ 1.1GB
100 KB 100000 5000 900 MB ~ 10.6 GB 690 MB ~ 10.4 GB
--- --- --- --- ---
1 MB 500 500 550 MB ~ 1 GB

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: clement2026
Once this PR has been reviewed and has the lgtm label, please assign ahrtr for approval. For more information see the Kubernetes Code Review Process.

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

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

@k8s-ci-robot
Copy link

Hi @clement2026. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Signed-off-by: Clement <gh.2lgqz@aleeas.com>
@clement2026 clement2026 force-pushed the v3_5-backport-DefaultSnapshotCount-10k branch from ca7d3d6 to 8aee45e Compare September 14, 2024 11:18
@clement2026 clement2026 marked this pull request as ready for review September 15, 2024 02:25
@ahrtr
Copy link
Member

ahrtr commented Sep 15, 2024

/ok-to-test

@serathius
Copy link
Member

Don't think we should change defaults in stable releases.

@ahrtr
Copy link
Member

ahrtr commented Sep 16, 2024

I don't insist on backporting the default value to 3.5, but it's a low hang fruit to improve the performance, and personally I do not see any risk (the only minor comment is about CPU usage change because snapshot is much more frequent against the old value) so should also be safe.

It's open to the community discussion.

@serathius
Copy link
Member

cc @liggitt who has a lot of experience about stability from K8s

@liggitt
Copy link
Contributor

liggitt commented Sep 17, 2024

Anyone who wants this benefit can already obtain it by explicitly setting --snapshot-count=..., right?

If this is already overrideable by operators, I would avoid changing defaults in a backport

@clement2026
Copy link
Contributor Author

clement2026 commented Sep 18, 2024

I ran some benchmarks for this PR and they show higher throughput(#17098 (comment)).

If we don’t have enough support to backport DefaultSnapshotCount =10_000 to v3.5, I'm okay with closing this PR and focusing on v3.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants