Skip to content

Overhauling TiKV RocksDB configuration file#5746

Merged
ti-chi-bot merged 6 commits into
pingcap:masterfrom
tabokie:patch-5
Jun 28, 2021
Merged

Overhauling TiKV RocksDB configuration file#5746
ti-chi-bot merged 6 commits into
pingcap:masterfrom
tabokie:patch-5

Conversation

@tabokie
Copy link
Copy Markdown
Contributor

@tabokie tabokie commented Jun 2, 2021

First-time contributors' checklist

What is changed, added or deleted? (Required)

writecf and lockcf only listed partial configuration items, and their selections aren't consistent.

Which TiDB version(s) do your changes apply to? (Required)

  • master (the latest development version)
  • v5.1 (TiDB 5.1 versions)
  • v5.0 (TiDB 5.0 versions)
  • v4.0 (TiDB 4.0 versions)
  • v3.1 (TiDB 3.1 versions)
  • v3.0 (TiDB 3.0 versions)
  • v2.1 (TiDB 2.1 versions)

What is the related PR or file 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

@ti-chi-bot ti-chi-bot requested a review from TomShawn June 2, 2021 07:09
@ti-chi-bot ti-chi-bot added missing-translation-status This PR does not have translation status info. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 2, 2021
@TomShawn
Copy link
Copy Markdown
Contributor

TomShawn commented Jun 2, 2021

@tabokie Could you please involve technical review(s)? Thanks~

@TomShawn TomShawn self-assigned this Jun 2, 2021
@TomShawn TomShawn added needs-cherry-pick-release-2.1 translation/doing This PR's assignee is translating this PR. labels Jun 2, 2021
@ti-chi-bot ti-chi-bot removed the missing-translation-status This PR does not have translation status info. label Jun 2, 2021
@TomShawn TomShawn added the requires-version-specific-changes After cherry-picked, the cherry-picked PR requires further changes. label Jun 2, 2021
@tabokie
Copy link
Copy Markdown
Contributor Author

tabokie commented Jun 2, 2021

This change isn't urgent (not due before next release). @yiwu-arbug can help take a look.

@tabokie
Copy link
Copy Markdown
Contributor Author

tabokie commented Jun 2, 2021

BTW I don't have the permission to request reviews in this repo.

@TomShawn
Copy link
Copy Markdown
Contributor

TomShawn commented Jun 2, 2021

BTW I don't have the permission to request reviews in this repo.

You can use the /cc @reviewer_id command to request a review.

@TomShawn
Copy link
Copy Markdown
Contributor

TomShawn commented Jun 2, 2021

/cc @yiwu-arbug

@ti-chi-bot ti-chi-bot requested a review from yiwu-arbug June 2, 2021 07:19
Comment thread tikv-configuration-file.md Outdated
## rocksdb.writecf

Configuration items related to `rocksdb.writecf`
Configuration items related to `rocksdb.writecf`, which are identical to `rocksdb.defaultcf`. Those not listed here have the same default value and are recommended to be set the same as `rocksdb.defaultcf`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Those not listed here have the same default value and are recommended to be set the same as rocksdb.defaultcf."

This may not be always true.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are two parts in this statement. I think at least we should make sure the first part stay true. For master branch I've checked and repaired all the violating items.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

then how about removing "and are recommended to be set the same as rocksdb.defaultcf."?

Comment thread tikv-configuration-file.md Outdated
Copy link
Copy Markdown
Contributor

@yiwu-arbug yiwu-arbug left a comment

Choose a reason for hiding this comment

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

rest looking good

@ti-chi-bot
Copy link
Copy Markdown
Member

@yiwu-arbug: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

Details

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 ti-community-infra/tichi repository.

Comment thread tikv-configuration-file.md Outdated
## rocksdb.writecf

Configuration items related to `rocksdb.writecf`
Configuration items related to `rocksdb.writecf`, which are identical to `rocksdb.defaultcf`. Those not listed here have the same default value as `rocksdb.defaultcf`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does it mean by "Those not listed here"? Are there any config items that are not documented here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are the same as rocksdb.defaultcf. Listing all of the duplicates would be cumbersome.

Copy link
Copy Markdown
Contributor

@TomShawn TomShawn Jun 4, 2021

Choose a reason for hiding this comment

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

I don't think so. Suppose that you want to see a TiKV config item under rocksdb.writecf but are told to see the config items under rocksdb.defaultcf (another section) because they are duplicated and, therefore, documented only in one place. Both document completeness and user experience are compromised. One possible solution to that is to document all the config items in one place but the title should incorporate both rocksdb.writecf and rocksdb.defaultcf. A note should also be given to explain why they are put together.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Make sense.

Comment thread tikv-configuration-file.md Outdated
+ The minimum SST file size when the compaction guard is enabled. This configuration prevents SST files from being too small when the compaction guard is enabled.
+ Default value: `"8MB"`
+ Unit: KB|MB|GB
Configuration items related to `rocksdb.lockcf`, which are identical to `rocksdb.defaultcf`. Those not listed here have the same default value as `rocksdb.defaultcf`.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ditto

@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 4, 2021
ti-chi-bot pushed a commit to ti-chi-bot/docs that referenced this pull request Jun 28, 2021
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
@ti-chi-bot
Copy link
Copy Markdown
Member

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

@ti-chi-bot
Copy link
Copy Markdown
Member

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

@ti-chi-bot
Copy link
Copy Markdown
Member

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

tabokie added a commit to tabokie/docs that referenced this pull request Jul 12, 2021
Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie added a commit to tabokie/docs that referenced this pull request Jul 12, 2021
Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie added a commit to tabokie/docs that referenced this pull request Jul 12, 2021
Signed-off-by: tabokie <xy.tao@outlook.com>
@TomShawn TomShawn assigned en-jin19 and unassigned TomShawn Jul 27, 2021
@en-jin19 en-jin19 added translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR. and removed translation/doing This PR's assignee is translating this PR. labels Aug 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-version-specific-changes After cherry-picked, the cherry-picked PR requires further changes. size/L Denotes a PR that changes 100-499 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. translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants