docs icon indicating copy to clipboard operation
docs copied to clipboard

restore: update the definition of the parameter --load-stats and the usage of pitr id map

Open Leavrth opened this issue 7 months ago • 2 comments

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.

  • [x] master (the latest development version)
  • [x] v9.0 (TiDB 9.0 versions)
  • [ ] v8.5 (TiDB 8.5 versions)
  • [ ] v8.4 (TiDB 8.4 versions)
  • [ ] v8.3 (TiDB 8.3 versions)
  • [ ] v8.1 (TiDB 8.1 versions)
  • [ ] v7.5 (TiDB 7.5 versions)
  • [ ] v7.1 (TiDB 7.1 versions)
  • [ ] v6.5 (TiDB 6.5 versions)
  • [ ] v6.1 (TiDB 6.1 versions)
  • [ ] v5.4 (TiDB 5.4 versions)

What is the related PR or file link(s)?

  • This PR is translated from: https://github.com/pingcap/docs-cn/pull/20346
  • Other reference link(s):

Do your changes match any of the following descriptions?

  • [ ] Delete files
  • [ ] Change aliases
  • [ ] Need modification after applied to another branch
  • [ ] Might cause conflicts after applied to another branch

Leavrth avatar May 30 '25 04:05 Leavrth

@gemini-code-assist[bot]: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

Code Review

Thank you for your contribution to the TiDB documentation! This pull request updates the documentation regarding the --load-stats parameter and the usage of the pitr_id_map, as well as introducing the new --fast-load-sys-tables parameter for v9.0. The changes improve the clarity and completeness of the documentation in these areas.

I've reviewed the changes against our documentation style guide and the standard review criteria. Overall, the changes are well-structured and technically accurate. I've identified a few areas for potential improvement, primarily related to avoiding repetition and slightly refining phrasing for clarity, as outlined in the review comments below.

Specifically, I've referenced the style guide's principles on avoiding unnecessary repetition [^1] and maintaining clarity/simplicity [^2].

Summary of Findings

  • Duplication of information about --fast-load-sys-tables: Information about the new --fast-load-sys-tables parameter, its mechanism, and the difference between physical and logical restore is duplicated across br-snapshot-guide.md and br-snapshot-manual.md (appearing twice in the latter). This violates the style guide's principle of avoiding unnecessary repetition [^1] and makes the documentation harder to maintain and read. It is recommended to consolidate this information into a single primary location and link to it from other relevant sections.
  • Informal phrasing: The phrase "at will" is used in two places to describe the risk of deleting pitr_id_map data/files. Using a more formal term like "arbitrarily" or "without understanding the implications" would improve clarity [^2].
  • Documentation structure and flow: The introduction of the --fast-load-sys-tables parameter in br-snapshot-manual.md could benefit from a clearer transition or a dedicated sub-heading to improve logical flow [^3].
  • Example clarity: An example command in br-snapshot-manual.md includes the --load-stats parameter, which is the default behavior, while the preceding note discusses the behavior when this parameter is set to false. This could potentially confuse readers. Clarifying the example or the note could improve understanding.

Merge Readiness

The pull request introduces valuable updates regarding restore parameters and pitr_id_map usage. However, the significant duplication of information about the --fast-load-sys-tables parameter is a high-severity issue that impacts the clarity and maintainability of the documentation. I recommend addressing this duplication and the other minor points before merging. I am unable to approve this pull request directly; please have other reviewers review and approve this code before merging.

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.

ti-chi-bot[bot] avatar May 30 '25 04:05 ti-chi-bot[bot]

@BornChanger: 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.

ti-chi-bot[bot] avatar Jun 26 '25 05:06 ti-chi-bot[bot]

[LGTM Timeline notifier]

Timeline:

  • 2025-07-03 02:05:12.239181744 +0000 UTC m=+1533364.962360726: :ballot_box_with_check: agreed by hfxsd.
  • 2025-07-14 07:45:32.559174077 +0000 UTC m=+2504185.282353058: :ballot_box_with_check: agreed by lilin90.

ti-chi-bot[bot] avatar Jul 14 '25 07:07 ti-chi-bot[bot]

This PR is ready for merging.

lilin90 avatar Jul 14 '25 07:07 lilin90

/unhold

lilin90 avatar Aug 19 '25 07:08 lilin90

/approve

lilin90 avatar Aug 19 '25 07:08 lilin90

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lilin90

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

The pull request process is described 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

ti-chi-bot[bot] avatar Aug 19 '25 07:08 ti-chi-bot[bot]