*: support send flashback cluster RPC #37659

Defined2014 · 2022-09-07T05:44:16Z

What problem does this PR solve?

Issue Number: close #37651, close #37665

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Support send flashback RPC.

ti-chi-bot · 2022-09-07T05:44:17Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

bb7133
zimulala

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.

Defined2014 · 2022-09-07T09:38:03Z

/run-check-issue-triage-complete

statistics/handle/ddl.go

sessionctx/variable/tidb_vars.go

ddl/cluster.go

Defined2014 · 2022-09-08T03:52:10Z

Updated, PTAL
@bb7133

sessionctx/variable/sysvar.go

bb7133

LGTM

ddl/cluster.go

zimulala · 2022-09-08T11:22:13Z

ddl/cluster.go

+		// A hack way to make global variables are synchronized to all tidb.
+		// TiKV will block read/write request during flashback cluster,
+		// So it's not very dangerous when sync failed.
+		time.Sleep(1 * time.Second)


Why here is 1s?

You can see the comments, just let most TiDB synced this global variable.

1s just a magic number. I found the global variable will synced by ETCD, so the time cost will be very small

zimulala · 2022-09-08T11:38:09Z

ddl/cluster.go

+			endKey = rangeEndKey
+		}
+
+		req := tikvrpc.NewRequest(tikvrpc.CmdFlashbackToVersion, &kvrpcpb.FlashbackToVersionRequest{


Do we need to set MaxExecutionDurationMs?

It will be set to timeout in client-go.
https://github.com/tikv/client-go/blob/426055b5755b4a64df7b92b278aee6e65479d53c/internal/locate/region_request.go#L962-L965

zimulala · 2022-09-08T11:59:12Z

ddl/cluster.go

+				job.State = model.JobStateCancelled
+				return ver, errors.Trace(err)
+			}
+			job.Args[gcEnabledArgsOffset] = &gcEnableValue


Why do we need to do it here? Or maybe we can do it in StateWriteOnly

we should store it in the first stage, like onRecoverTable func.

tidb/ddl/table.go

Lines 430 to 453 in e75a079

// 1. Check GC enable status, to decided whether enable GC after recover table.

// a. Why not disable GC before put the job to DDL job queue?

// Think about concurrency problem. If a recover job-1 is doing and already disabled GC,

// then, another recover table job-2 check GC enable will get disable before into the job queue.

// then, after recover table job-2 finished, the GC will be disabled.

// b. Why split into 2 steps? 1 step also can finish this job: check GC -> disable GC -> recover table -> finish job.

// What if the transaction commit failed? then, the job will retry, but the GC already disabled when first running.

// So, after this job retry succeed, the GC will be disabled.

// 2. Do recover table job.

// a. Check whether GC enabled, if enabled, disable GC first.

// b. Check GC safe point. If drop table time if after safe point time, then can do recover.

// otherwise, can't recover table, because the records of the table may already delete by gc.

// c. Remove GC task of the table from gc_delete_range table.

// d. Create table and rebase table auto ID.

// e. Finish.

switch tblInfo.State {

case model.StateNone:

// none -> write only

// check GC enable and update flag.

if gcEnable {

job.Args[checkFlagIndexInJobArgs] = recoverTableCheckFlagEnableGC

} else {

job.Args[checkFlagIndexInJobArgs] = recoverTableCheckFlagDisableGC

}

OK, but I think putting it to model.StateWriteOnly is also OK. But it should be a little bit easier to understand here

zimulala · 2022-09-09T03:00:32Z

ddl/cluster_test.go

+			assert.Equal(t, tk.ResultSetToResult(rs, "").Rows()[0][1], variable.On)
+			rs, err = tk.Exec("show variables like 'tidb_gc_enable'")
+			assert.NoError(t, err)
+			assert.Equal(t, tk.ResultSetToResult(rs, "").Rows()[0][1], variable.Off)


How about user do set global tidb_gc_enable = on" in this state?

This is an illegal operation, we will add it into doc

zimulala

LGTM

zimulala · 2022-09-09T03:18:19Z

/merge

ti-chi-bot · 2022-09-09T03:18:23Z

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

Commit hash: 8f4bc12

sre-bot · 2022-09-09T04:15:26Z

TiDB MergeCI notify

🔴 Bad News! [2] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🔴 failed 2, success 15, total 17	17 min	Existing failure
idc-jenkins-ci-tidb/integration-ddl-test	🔴 failed 1, success 5, total 6	13 min	Existing failure
idc-jenkins-ci/integration-cdc-test	🟢 all 37 tests passed	29 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	15 min	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	6 min 15 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	4 min 54 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	4 min 42 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	3 min 57 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	3 min 3 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

Defined2014 added 3 commits September 6, 2022 17:45

*: flashback support send RPC

8ed1f0f

add some tests

7465000

Merge branch 'master' into flashback-send-rpc

0d11414

Defined2014 requested a review from a team as a code owner September 7, 2022 05:44

Defined2014 added 4 commits September 7, 2022 13:59

fix fmt

c653c51

fix fmt

e924564

fix fmt

b2409e3

fix test

54b8f81

Defined2014 requested review from bb7133, zimulala, tangenta and xhebox September 7, 2022 07:27

ti-chi-bot added the do-not-merge/needs-triage-completed label Sep 7, 2022

ti-chi-bot removed the do-not-merge/needs-triage-completed label Sep 7, 2022

bb7133 reviewed Sep 7, 2022

View reviewed changes

statistics/handle/ddl.go Show resolved Hide resolved

bb7133 reviewed Sep 7, 2022

View reviewed changes

sessionctx/variable/tidb_vars.go Outdated Show resolved Hide resolved

bb7133 reviewed Sep 7, 2022

View reviewed changes

ddl/cluster.go Outdated Show resolved Hide resolved

bb7133 reviewed Sep 7, 2022

View reviewed changes

ddl/cluster.go Outdated Show resolved Hide resolved

Defined2014 added 2 commits September 8, 2022 11:27

follow comments

29d46d4

update

411885b

zimulala reviewed Sep 8, 2022

View reviewed changes

sessionctx/variable/sysvar.go Show resolved Hide resolved

update

2ded02f

bb7133 approved these changes Sep 8, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 8, 2022

zimulala reviewed Sep 8, 2022

View reviewed changes

update comments

8f4bc12

zimulala reviewed Sep 9, 2022

View reviewed changes

zimulala approved these changes Sep 9, 2022

View reviewed changes

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 Sep 9, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 9, 2022

Merge branch 'master' into flashback-send-rpc

250258f

ti-chi-bot merged commit d86e795 into pingcap:master Sep 9, 2022

Defined2014 deleted the flashback-send-rpc branch September 9, 2022 03:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: support send flashback cluster RPC #37659

*: support send flashback cluster RPC #37659

Defined2014 commented Sep 7, 2022 •

edited

Loading

ti-chi-bot commented Sep 7, 2022 •

edited

Loading

Defined2014 commented Sep 7, 2022

Defined2014 commented Sep 8, 2022

bb7133 left a comment

zimulala Sep 8, 2022

Defined2014 Sep 8, 2022

Defined2014 Sep 8, 2022

zimulala Sep 9, 2022

zimulala Sep 8, 2022

Defined2014 Sep 8, 2022

zimulala Sep 8, 2022

Defined2014 Sep 8, 2022 •

edited

Loading

zimulala Sep 9, 2022

zimulala Sep 9, 2022

Defined2014 Sep 9, 2022

zimulala left a comment

zimulala commented Sep 9, 2022

ti-chi-bot commented Sep 9, 2022

sre-bot commented Sep 9, 2022

	// 1. Check GC enable status, to decided whether enable GC after recover table.
	// a. Why not disable GC before put the job to DDL job queue?
	// Think about concurrency problem. If a recover job-1 is doing and already disabled GC,
	// then, another recover table job-2 check GC enable will get disable before into the job queue.
	// then, after recover table job-2 finished, the GC will be disabled.
	// b. Why split into 2 steps? 1 step also can finish this job: check GC -> disable GC -> recover table -> finish job.
	// What if the transaction commit failed? then, the job will retry, but the GC already disabled when first running.
	// So, after this job retry succeed, the GC will be disabled.
	// 2. Do recover table job.
	// a. Check whether GC enabled, if enabled, disable GC first.
	// b. Check GC safe point. If drop table time if after safe point time, then can do recover.
	// otherwise, can't recover table, because the records of the table may already delete by gc.
	// c. Remove GC task of the table from gc_delete_range table.
	// d. Create table and rebase table auto ID.
	// e. Finish.
	switch tblInfo.State {
	case model.StateNone:
	// none -> write only
	// check GC enable and update flag.
	if gcEnable {
	job.Args[checkFlagIndexInJobArgs] = recoverTableCheckFlagEnableGC
	} else {
	job.Args[checkFlagIndexInJobArgs] = recoverTableCheckFlagDisableGC
	}

*: support send flashback cluster RPC #37659

*: support send flashback cluster RPC #37659

Conversation

Defined2014 commented Sep 7, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 7, 2022 • edited Loading

Defined2014 commented Sep 7, 2022

Defined2014 commented Sep 8, 2022

bb7133 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Defined2014 Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala left a comment

Choose a reason for hiding this comment

zimulala commented Sep 9, 2022

ti-chi-bot commented Sep 9, 2022

sre-bot commented Sep 9, 2022

TiDB MergeCI notify

Defined2014 commented Sep 7, 2022 •

edited

Loading

ti-chi-bot commented Sep 7, 2022 •

edited

Loading

Defined2014 Sep 8, 2022 •

edited

Loading