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

ddl: defining the placement rule of a leader requires REPLICAS option #20813

Merged
merged 10 commits into from
Nov 5, 2020
Merged

ddl: defining the placement rule of a leader requires REPLICAS option #20813

merged 10 commits into from
Nov 5, 2020

Conversation

TszKitLo40
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #20751

Problem Summary:

What is changed and how it works?

What's Changed:
defining the placement rule of a leader requires REPLICAS option
How it Works:
If the role is leader, set the replicas to 1

Check List

Tests

  • Unit test

Release note

  • No release note

@TszKitLo40 TszKitLo40 requested a review from a team as a code owner November 3, 2020 13:02
@TszKitLo40 TszKitLo40 requested review from XuHuaiyu and removed request for a team November 3, 2020 13:02
@github-actions github-actions bot added sig/sql-infra SIG: SQL Infra sig/execution SIG execution labels Nov 3, 2020
djshow832
djshow832 reviewed Nov 4, 2020
View reviewed changes
ddl/ddl_api.go
Comment on lines +5813 to +5815
if spec.Replicas == 0 {
spec.Replicas = 1
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the user sets it to 0 explicitly?
e.g. leader=role replicas=0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case has been treated in the parser.

Copy link
Contributor

Choose a reason for hiding this comment

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

why not just throw "Invalid placement option REPLICAS, it is should be identified"? @djshow832

Copy link
Contributor

Choose a reason for hiding this comment

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

The number of leaders can be only 1. It's unnecessary to declare it.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@djshow832 djshow832 removed the sig/execution SIG execution label Nov 4, 2020
@djshow832 djshow832 requested review from tangenta and removed request for XuHuaiyu November 4, 2020 02:34
djshow832
djshow832 reviewed Nov 4, 2020
View reviewed changes
ddl/placement_sql_test.go Outdated Show resolved Hide resolved
TszKitLo40 and others added 2 commits November 4, 2020 11:18
@TszKitLo40 @djshow832
Update ddl/placement_sql_test.go
7751a09 
Co-authored-by: djshow832 <zhangming@pingcap.com>
@AilinKid
Merge branch 'master' into fix20751
5277318
djshow832
djshow832 reviewed Nov 4, 2020
View reviewed changes
Copy link
Contributor

@djshow832 djshow832 left a comment

Choose a reason for hiding this comment

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

LGTM

ddl/ddl_api.go
Comment on lines +5813 to +5815
if spec.Replicas == 0 {
spec.Replicas = 1
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The number of leaders can be only 1. It's unnecessary to declare it.

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 4, 2020
AilinKid
AilinKid reviewed Nov 4, 2020
View reviewed changes
Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Nov 4, 2020
ti-srebot
ti-srebot previously approved these changes Nov 4, 2020
View reviewed changes
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Nov 4, 2020
@pingcap pingcap deleted a comment from ti-challenge-bot bot Nov 4, 2020
@pingcap pingcap deleted a comment from ti-challenge-bot bot Nov 4, 2020
wjhuang2016
wjhuang2016 previously approved these changes Nov 4, 2020
View reviewed changes
Copy link
Member

@wjhuang2016 wjhuang2016 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Nov 4, 2020
@wjhuang2016
Copy link
Member

/reward 600

@ti-challenge-bot
Copy link

You are not the mentor for the linked issue.

@wjhuang2016
Copy link
Member

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 4, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@TszKitLo40 merge failed.

@TszKitLo40 TszKitLo40 dismissed stale reviews from wjhuang2016 and ti-srebot via a3a8837 November 4, 2020 07:41
@wjhuang2016
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@TszKitLo40 merge failed.

@wjhuang2016
Copy link
Member

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@djshow832
Copy link
Contributor

/reward 600

@ti-challenge-bot
Copy link

Reward success.

@ti-srebot ti-srebot merged commit 657ad73 into pingcap:master Nov 5, 2020
@ti-challenge-bot
Copy link

@TszKitLo40, Congratulations, you get 600 in this PR, and your total score is 1200 in challenge program.

@xhebox xhebox mentioned this pull request Jul 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AilinKid AilinKid AilinKid left review comments

@djshow832 djshow832 djshow832 approved these changes

@ti-srebot ti-srebot ti-srebot left review comments

@wjhuang2016 wjhuang2016 wjhuang2016 left review comments

@tangenta tangenta Awaiting requested review from tangenta

Assignees
No one assigned
Labels
sig/sql-infra SIG: SQL Infra status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ddl: defining the placement rule of a leader requires REPLICAS option
5 participants
@TszKitLo40 @wjhuang2016 @ti-srebot @djshow832 @AilinKid