-
Notifications
You must be signed in to change notification settings - Fork 689
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
add saas scenario best practices #20668
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
@Frank945946: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this: 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. |
Co-authored-by: Frank945946 <[email protected]>
@lilinghai: adding LGTM is restricted to approvers and reviewers in OWNERS files. In response to this: 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. |
Co-authored-by: lilinghai <[email protected]>
…nto saas-best-practices
/bot-review |
@@ -401,6 +401,7 @@ | |||
- [Local Read Under Three Data Centers Deployment](/best-practices/three-dc-local-read.md) | |||
- [Use UUIDs](/best-practices/uuid.md) | |||
- [Read-Only Storage Nodes](/best-practices/readonly-nodes.md) | |||
- [SaaS Multi-Tenant Scenarios](/best-practices/saas-best-practices.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title of the new document is not clear enough. It is recommended to clarify what the best practices are about.
- [SaaS Multi-Tenant Scenarios](/best-practices/saas-best-practices.md) | |
- [Best Practices for SaaS Multi-Tenant Scenarios](/best-practices/saas-best-practices.md) |
✅ AI review completed, 1 comments generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
|
||
This document introduces best practices for TiDB in SaaS (Software as a Service) multi-tenant environments, especially in scenarios where the **number of tables in a single cluster exceeds one million**. By making reasonable configurations and choices, you can enable TiDB to run efficiently and stably in SaaS scenarios while reducing resource consumption and costs. | ||
|
||
> **Note:** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an inconsistency with the version recommendations; while the note on line 12 suggests using TiDB v8.5.0 or later, other sections mention features available from TiDB v8.4.0. Consider aligning these version recommendations for clarity.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request adds a new best practices document for SaaS multi-tenant scenarios. The document provides valuable guidance on hardware requirements, configuration, and optimization techniques for running TiDB in such environments. The addition of this document to the TOC is also appropriate. Overall, this is a good contribution to the documentation.
Summary of Findings
- Missing links to specific configuration items: The document mentions several TiDB configuration items but does not provide direct links to their documentation. Adding these links would improve the user experience.
- Inconsistent formatting: There are some inconsistencies in formatting, such as the use of different bullet point styles and spacing. Standardizing the formatting would improve readability.
- Missing context for hardware recommendations: The hardware recommendations for TiKV and PD are provided without much context. It would be helpful to explain the rationale behind these recommendations and how they relate to the SaaS multi-tenant scenario.
Merge Readiness
The pull request introduces a valuable best practices document. However, addressing the identified issues related to missing links and formatting inconsistencies would further enhance the document's quality and user-friendliness. I am unable to approve this pull request, and recommend that another reviewer also reviews this code before merging.
* [`tidb_auto_build_stats_concurrency`](/system-variables.md#tidb_auto_build_stats_concurrency-new-in-v650) and [`tidb_build_sampling_stats_concurrency`](/system-variables.md#tidb_build_sampling_stats_concurrency-new-in-v750) affect the concurrency of TiDB statistics construction and should be adjusted based on the scenario: | ||
- For scenarios with many partitioned tables, prioritize increasing the value of `tidb_auto_build_stats_concurrency`. | ||
- For scenarios with many columns, prioritize increasing the value of `tidb_build_sampling_stats_concurrency`. | ||
* The product of the three values of `tidb_auto_analyze_concurrency`, `tidb_auto_build_stats_concurrency` and `tidb_build_sampling_stats_concurrency` should not exceed the number of TiDB CPU cores to avoid excessive resource usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For 1 million tables, use BR instances with 32 GiB or more memory. | ||
- For 3 million tables, use BR instances with 64 GiB or more memory. | ||
* BR log backup and snapshot recovery consume additional TiKV memory. It is recommended to use TiKV with 32 GiB or higher memory specifications. | ||
* Adjust the BR configurations [`pitr-batch-count` and `pitr-concurrency`](/br/use-br-command-line-tool.md#common-options) as needed to improve BR log recovery speed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First-time contributors' checklist
What is changed, added or deleted? (Required)
Which TiDB version(s) do your changes apply to? (Required)
Tips for choosing the affected version(s):
By default, CHOOSE MASTER ONLY so your changes will be applied to the next TiDB major or minor releases. If your PR involves a product feature behavior change or a compatibility change, CHOOSE THE AFFECTED RELEASE BRANCH(ES) AND MASTER.
For details, see tips for choosing the affected versions.
What is the related PR or file link(s)?
Do your changes match any of the following descriptions?