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.

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

*: support the TiFlash replica of table #12453

Merged
merged 27 commits into from
Oct 29, 2019

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Sep 27, 2019

What problem does this PR solve?

Related parser PR: pingcap/parser#564

  • Add DDL syntax to support add table flash replica.
ALTER TABLE [table_name] SET TIFLASH REPLICA [count] LOCATION LABELS [location_labels]
eg:
ALTER TABLE t SET TIFLASH REPLICA 3 LOCATION LABELS "rack", "host", "abc";
  • Add /flash/replica http api to query the all table flash replica infos.
▶ curl "http://$IP:10080/tiflash/replica"
[
 {
  "id": 84,
  "replica_count": 3,
  "location_labels": [
   "rack",
   "host",
   "abc"
  ],
  "status": false
 }
]% 
  • The /flash/replicaHTTP API also use to let TiFlash to report the table replica status.
# update the replica status.
▶ curl -X POST -d '{"id":84,"region_count":3,"flash_region_count":3}' http://127.0.0.1:10080/tiflash/replica

▶ curl http://127.0.0.1:10080/tiflash/replica
[
 {
  "id": 84,
  "replica_count": 3,
  "location_labels": [
   "rack",
   "host",
   "abc"
  ],
  "status": true    # the status was true mean the flash replica is available now.
 }
]% 

What is changed and how it works?

The flash replica information is store in the tableInfo, So when set/update the table flash replica information, I use DDL to change the tableInfo.

Check List

Tests

  • Unit test

  • Integration test

  • Manual test (add detailed scripts or steps below)
    Code changes

  • Has exported function/method change

Side effects

Related changes

Release note

@codecov
Copy link

codecov bot commented Sep 27, 2019

Codecov Report

Merging #12453 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12453   +/-   ##
===========================================
  Coverage   80.2271%   80.2271%           
===========================================
  Files           468        468           
  Lines        110303     110303           
===========================================
  Hits          88493      88493           
  Misses        15085      15085           
  Partials       6725       6725

@zyxbest
Copy link
Contributor

zyxbest commented Sep 28, 2019

/run-all-tests

@zyxbest
Copy link
Contributor

zyxbest commented Sep 28, 2019

/rebuild

@lzmhhh123 lzmhhh123 self-requested a review October 11, 2019 07:47
server/http_handler.go Outdated Show resolved Hide resolved
ddl/ddl_api.go Outdated Show resolved Hide resolved
server/http_handler_test.go Outdated Show resolved Hide resolved
server/http_handler_test.go Outdated Show resolved Hide resolved
ddl/db_test.go Show resolved Hide resolved
ddl/ddl_api.go Show resolved Hide resolved
if err != nil {
return errors.Trace(infoschema.ErrTableNotExists.GenWithStackByArgs(ident.Schema, ident.Name))
}

Copy link
Contributor

Choose a reason for hiding this comment

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

How about alter table t_flash set tiflash replica 2 location labels 'a','a'; ?
If labels are the same, is it OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's OK. PD will get the same labels and deduplicate.

ddl/table.go Show resolved Hide resolved
@crazycs520
Copy link
Contributor Author

/run-all-tests

@bb7133 bb7133 removed the status/DNM label Oct 29, 2019
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

@bb7133 bb7133 added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 29, 2019
@bb7133
Copy link
Member

bb7133 commented Oct 29, 2019

PTAL @zimulala @AilinKid

ddl/ddl_api.go Outdated Show resolved Hide resolved
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.

Rest LGTM

tables := schema.SchemaTables(db.Name)
for _, tbl := range tables {
tblInfo := tbl.Meta()
if tblInfo.TiFlashReplica == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

So TiFlashReplica is in tableInfo, what will happen if tableInfo loses this meta, for example, admin repair table will reconstruct tableInfo from create table stmt without TiFlashReplica info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

em..., Great question, I have not consider this before...

zimulala
zimulala previously approved these changes Oct 29, 2019
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@crazycs520
Copy link
Contributor Author

/run-unit-test

@AilinKid
Copy link
Contributor

AilinKid commented Oct 29, 2019

LGTM
additional consideration:
admin repair table could not repair table info with TiFlashReplica by now, cause create table stmt don't have TiFlashReplica info. Maybe it's not good choice to add it in tableInfo, I will add todo in my PR #12046 comment, please take care of this.

@AilinKid AilinKid added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Oct 29, 2019
@crazycs520 crazycs520 merged commit a8ed950 into pingcap:master Oct 29, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
lzmhhh123 pushed a commit to lzmhhh123/tidb that referenced this pull request Jan 19, 2020
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
july2993 added a commit to pingcap/tidb-binlog that referenced this pull request Apr 3, 2020
test:
1. create table t(id int);
2. ALTER TABLE t SET TIFLASH REPLICA 3 LOCATION LABELS "rack", "host", "abc";            // model.ActionSetTiFlashReplica
3. curl -X POST -d '{"id":45,"region_count":3,"flash_region_count":3}' https://127.0.0.1:10080/tiflash/replica  // model.ActionUpdateTiFlashReplicaStatus
4. performance write on table t and check can still replicate.

ref: pingcap/tidb#12453
july2993 added a commit to pingcap/tidb-binlog that referenced this pull request Apr 3, 2020
* Skip the tiflash job.

test:
1. create table t(id int);
2. ALTER TABLE t SET TIFLASH REPLICA 3 LOCATION LABELS "rack", "host", "abc";            // model.ActionSetTiFlashReplica
3. curl -X POST -d '{"id":45,"region_count":3,"flash_region_count":3}' https://127.0.0.1:10080/tiflash/replica  // model.ActionUpdateTiFlashReplicaStatus
4. performance write on table t and check can still replicate.

ref: pingcap/tidb#12453

* Specify status port for tidb

* Try repl SetReplicate

* check len
sre-bot pushed a commit to sre-bot/tidb-binlog that referenced this pull request Apr 3, 2020
test:
1. create table t(id int);
2. ALTER TABLE t SET TIFLASH REPLICA 3 LOCATION LABELS "rack", "host", "abc";            // model.ActionSetTiFlashReplica
3. curl -X POST -d '{"id":45,"region_count":3,"flash_region_count":3}' https://127.0.0.1:10080/tiflash/replica  // model.ActionUpdateTiFlashReplicaStatus
4. performance write on table t and check can still replicate.

ref: pingcap/tidb#12453
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/sql-infra SIG: SQL Infra status/LGT3 The PR has already had 3 LGTM. type/new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants