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.

Sign up for GitHub

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

Jump to bottom

docs: add the design of table attributes #28971

Merged
merged 10 commits into from
Nov 8, 2021
Merged

docs: add the design of table attributes #28971

merged 10 commits into from
Nov 8, 2021

Conversation

rleungx
Copy link
Member

@rleungx rleungx commented Oct 20, 2021

What problem does this PR solve?

Issue Number: ref tikv/pd#3839

Problem Summary: This PR adds the design of region label in SQL.

What is changed and how it works?

Only design document.

Check List

Tests

  • No code

Release note

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Oct 20, 2021 •
edited
Loading

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • disksing
  • xhebox

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.

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-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2021
@rleungx
Copy link
Member Author

rleungx commented Oct 20, 2021

/cc @nolouch @xhebox @disksing @Yisaer

nolouch
nolouch reviewed Oct 20, 2021
View reviewed changes
docs/design/2021-10-20-region-label-in-sql.md Outdated

## Background

Previously, we have designed a new mapping mechanism to establish a relationship between tables/partitions and regions which allows the user to specify the attributes to instruct the scheduling of PD.
Copy link
Member

Choose a reason for hiding this comment

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

Could you add more about attributes usage and the attributes currently planned to support?

@rleungx rleungx force-pushed the region-label-SQL branch 2 times, most recently from 0e510df to df0166e Compare October 20, 2021 09:43
disksing
disksing reviewed Oct 25, 2021
View reviewed changes
docs/design/2021-10-20-region-label-in-sql.md Outdated

## Introduction

Recently, we have encountered such a case that the scattered region of a table/partition is merged before inserting the data. We cannot prevent it from happening right now since we are not able to control the scheduling behavior at the table/partition level. However, it will sometimes lead to a serious hotspot problem. Under this circumstance, we are going to support the attributes for the table/partition so that we can handle this case.
Copy link
Contributor

Choose a reason for hiding this comment

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

This part looks more like the Background.

docs/design/2021-10-20-region-label-in-sql.md Outdated
@@ -0,0 +1,104 @@
# Proposal: Region Label in SQL
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe name it table/partition attributes?

@rleungx rleungx force-pushed the region-label-SQL branch 4 times, most recently from 4bcf9eb to bd3aa2c Compare October 28, 2021 09:10
@rleungx rleungx changed the title docs: add the design of region label in sql docs: add the design of table and partition attributes Oct 28, 2021
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 29, 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 Oct 29, 2021
@rleungx rleungx requested a review from nolouch October 29, 2021 10:13
@rleungx rleungx changed the title docs: add the design of table and partition attributes docs: add the design of table attributes Nov 3, 2021
@rleungx rleungx requested a review from kennytm November 3, 2021 07:49
kennytm
kennytm reviewed Nov 4, 2021
View reviewed changes
Copy link
Contributor

@kennytm kennytm left a comment

Choose a reason for hiding this comment

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

i think we need a /*T! */ feature like placement rules (https://github.com/pingcap/tidb/blob/master/docs/design/2020-06-24-placement-rules-in-sql.md)

rest lgtm.

docs/design/2021-10-20-table-attributes.md Outdated Show resolved Hide resolved
docs/design/2021-10-20-table-attributes.md Show resolved Hide resolved
docs/design/2021-10-20-table-attributes.md Outdated Show resolved Hide resolved
docs/design/2021-10-20-table-attributes.md Outdated Show resolved Hide resolved
@rleungx
Copy link
Member Author

rleungx commented Nov 4, 2021

i think we need a /*T! */ feature like placement rules (https://github.com/pingcap/tidb/blob/master/docs/design/2020-06-24-placement-rules-in-sql.md)

rest lgtm.

Only create table clause has /*T! */ feature in placement rules. Alter clause doesn't have it.

@rleungx rleungx requested a review from kennytm November 5, 2021 02:20
@kennytm
Copy link
Contributor

kennytm commented Nov 5, 2021

@rleungx ❓

@rleungx
Copy link
Member Author

rleungx commented Nov 5, 2021 •
edited
Loading

@rleungx ❓

I replied to the comment in the morning: #28971 (comment)

@rleungx
the design of region label in sql
34f574f 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address comments
d586e04 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address comments
72249e2 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
rename the feature name
230cf14 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address comments
2834c12 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
address comments
620b1b1 
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@xhebox
Copy link
Contributor

xhebox commented Nov 8, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 620b1b1

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

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

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 ti-chi-bot merged commit 74eaa06 into pingcap:master Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@disksing disksing disksing approved these changes

@nolouch nolouch nolouch approved these changes

@xhebox xhebox xhebox approved these changes

@kennytm kennytm kennytm approved these changes

@Yisaer Yisaer Awaiting requested review from Yisaer

Assignees
No one assigned
Labels
release-note-none Denotes a PR that doesn't merit a release note. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rleungx @ti-chi-bot @kennytm @xhebox @nolouch @disksing