Skip to content

storage:change configuration storage.max-key-size default value from 4096 to 8192#11331

Merged
youjiali1995 merged 7 commits intotikv:masterfrom
TonsnakeLin:master
Dec 1, 2021
Merged

storage:change configuration storage.max-key-size default value from 4096 to 8192#11331
youjiali1995 merged 7 commits intotikv:masterfrom
TonsnakeLin:master

Conversation

@TonsnakeLin
Copy link
Contributor

@TonsnakeLin TonsnakeLin commented Nov 10, 2021

What problem does this PR solve?

Issue Number: close #11241

Problem Summary:
Create a table from tidb-server successfully, but insert data failed if it has the length over 4096. Set the default value of storage.max-key-size to 8192 and It insert data successfully.

What is changed and how it works?

What's Changed:
Change the default value of storage.max-key-size from 4k to 8k.

Related changes

  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

Release note 

Please add a release note.
If you don't think this PR needs a release note then fill it with None.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 10, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • sticnarf
  • youjiali1995

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.

Details

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. do-not-merge/invalid-commit-message contribution This PR is from a community contributor. first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. labels Nov 10, 2021
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Nov 10, 2021

Welcome @TonsnakeLin!

It looks like this is your first PR to tikv/tikv 🎉.

I'm the bot to help you request reviewers, add labels and more, See available commands.

We want to make sure your contribution gets all the attention it needs!



Thank you, and welcome to tikv/tikv. 😃

@ti-chi-bot ti-chi-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 10, 2021
@TonsnakeLin
storage:change configuration storage.max-key-size default value from …
8417c4c 
…4096 to 8192

Signed-off-by: TonsnakeLin <lpbgytong@163.com>
@TonsnakeLin
Copy link
Contributor Author

TonsnakeLin commented Nov 11, 2021

/run-test

3 similar comments
@TonsnakeLin
Copy link
Contributor Author

TonsnakeLin commented Nov 11, 2021

/run-test

@TonsnakeLin
Copy link
Contributor Author

TonsnakeLin commented Nov 11, 2021

/run-test

@TonsnakeLin
Copy link
Contributor Author

TonsnakeLin commented Nov 11, 2021

/run-test

@sticnarf sticnarf mentioned this pull request Nov 12, 2021
Merged
12 tasks
youjiali1995
youjiali1995 reviewed Nov 12, 2021
View reviewed changes
Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

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

Should change the value in test_serde_custom_tikv_config and tests/integrations/config/test-custom.toml.

tikv/tests/integrations/config/mod.rs

Line 615 in 999ca55

max_key_size: 8192,

@youjiali1995
Copy link
Contributor

youjiali1995 commented Nov 12, 2021

@TonsnakeLin You have to use git commit -s --amend to modify your last commit message to contain close #11241 and add a signoff.

@TonsnakeLin
Copy link
Contributor Author

TonsnakeLin commented Nov 16, 2021

Should change the value in test_serde_custom_tikv_config and tests/integrations/config/test-custom.toml.

tikv/tests/integrations/config/mod.rs

Line 615 in 999ca55

max_key_size: 8192,

got it！

@TonsnakeLin
Copy link
Contributor Author

TonsnakeLin commented Nov 17, 2021

/run-test

2 similar comments
@TonsnakeLin
Copy link
Contributor Author

TonsnakeLin commented Nov 18, 2021

/run-test

@TonsnakeLin
Copy link
Contributor Author

TonsnakeLin commented Nov 18, 2021

/run-test

@TonsnakeLin
Copy link
Contributor Author

TonsnakeLin commented Nov 29, 2021

@youjiali1995 @sticnarf please review or approve, thanks

@sticnarf
Copy link
Contributor

sticnarf commented Nov 29, 2021

Please change

tikv/tests/integrations/config/test-custom.toml

Line 83 in 225bbdd

max-key-size = 8192
to a different value and also change the related tests. Otherwise, we cannot verify the config can be customized successfully.

@MyonKeminta
Copy link
Contributor

MyonKeminta commented Nov 30, 2021

Should change the value in test_serde_custom_tikv_config and tests/integrations/config/test-custom.toml.

tikv/tests/integrations/config/mod.rs

Line 615 in 999ca55

max_key_size: 8192,

Looks that this comment is still not addressed?

@TonsnakeLin
storage:change max-key-size from 8192 to 4092 in test-custom.toml
0412a01 
close #11241

Signed-off-by: TonsnakeLin <lpbgytong@163.com>
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 1, 2021
@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 Dec 1, 2021
@sticnarf
Copy link
Contributor

sticnarf commented Dec 1, 2021

/merge

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 1, 2021

@sticnarf: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and 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.

If you have any questions about the PR merge process, please refer to pr process.

Details

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.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 1, 2021

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

DetailsCommit hash: 0412a01

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

ti-chi-bot commented Dec 1, 2021

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

Details

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.

@youjiali1995 youjiali1995 merged commit e606021 into tikv:master Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youjiali1995 youjiali1995 youjiali1995 approved these changes

@sticnarf sticnarf sticnarf approved these changes

@longfangsong longfangsong Awaiting requested review from longfangsong

@MyonKeminta MyonKeminta Awaiting requested review from MyonKeminta

Assignees

No one assigned

Labels

contribution This PR is from a community contributor. first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 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

No milestone

Development

Successfully merging this pull request may close these issues.

Relax default max key size

5 participants

@TonsnakeLin @ti-chi-bot @youjiali1995 @sticnarf @MyonKeminta