ddl, session: fix re-upgrade issues #44469

zimulala · 2023-06-07T04:01:52Z

What problem does this PR solve?

Issue Number: close #44158, and related #44366

Problem Summary:
Mock： do upgradeNewVer： Add a new mysql.table(mock system DDL)

case 1

The new TiDB upgrade failed(such as TiKV network problems), and the mock system DDL is queuing
Re-upgrade the TiDB(This operation will pause all DDL jobs by the system). It will pause the mock system DDL which always goes into runDDLJob.

case 2

The new TiDB upgrade failed(such as TiKV network problems), and the etcd owner path isn't remove
The etcd owner path TTL is 60s, we retry get owner info is 2s. So we can't get the owner's info normally.

What is changed and how it works?

Make all paused DDL jobs can't be runnable(won't go into runDDLJob) when upgrading state.
Extend the timeout to get owner's info.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
case 2.
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.

None

Defined2014

Could update job2SchemaNames function

--- a/ddl/job_table.go
+++ b/ddl/job_table.go
@@ -561,12 +561,12 @@ func job2SchemaNames(job *model.Job) []string {
                names = append(names, strings.ToLower(job.SchemaName))
                names = append(names, oldSchemaName.O)
                return names
-       case model.ActionRenameTables:
-               // TODO: Get this action's schema names.
-       case model.ActionExchangeTablePartition:
+       case model.ActionRenameTables, model.ActionExchangeTablePartition:
                // TODO: Get this action's schema names.
+               return nil
+       default:
+               return []string{job.SchemaName}
        }
-       return []string{job.SchemaName}
 }

session/mock_bootstrap.go

session/bootstraptest/bootstrap_upgrade_test.go

session/bootstrap.go

ddl/job_table.go

dhysum · 2023-06-12T02:22:09Z

ddl/job_table.go

 	}
+	// TODO: consider about model.ActionRenameTables and model.ActionExchangeTablePartition, which need to get the schema names.


Better one.

If not necessary, we'd better not add the code. "// TODO:" is a option.

It would be much better to talk about such kind of potential possibility in the design procedure, as much as possible.

Yeah, but here I think it needs to be added.

dhysum · 2023-06-12T02:54:47Z

session/bootstraptest/bootstrap_upgrade_test.go

+	m := meta.NewMeta(txn)
+	err = m.FinishBootstrap(session.CurrentBootstrapVersion - 1)
+	require.NoError(t, err)
+	err = txn.Commit(context.Background())


Shall we use the session but not txn here, or another session? It would be better use the same mechamism in certain routing.

This is because this method of FinishBootstrap call requires txn.

dhysum · 2023-06-12T02:59:53Z

session/bootstraptest/bootstrap_upgrade_test.go

+		}
+		if job.State == model.JobStatePaused && jobID == 0 {
+			// Mock pause the ddl job by system.
+			job.AdminOperator = model.AdminCommandBySystem


It looks like that would be better to do the real upgrade but not mock since it will be more like user's action.

And, the mock here normally seems being a litlle bit complex since there are cases that we may not know what will be going to happen in the future because the behavior of 'job.AdminOperator' may be changed by its module. In another word, the case here involves the inside-logic of 'admin pause' which may introduce unknow error in future.

Yes, but the test should fail if the internal implementation changes, so just modify the test. And the way I've come up with mocks so far is this. Do you have any other suggestions?

dhysum · 2023-06-12T03:01:56Z

session/mock_bootstrap.go

+	}
+	mustExecute(s, "use mysql")
+	mustExecute(s, `create table if not exists mock_sys_t(
+		c1 int, c2 int, c3 int, c11 tinyint, index fk_c1(c1)


It would be better that different column is with different kind of type

This is a simple test, hopefully a system DDL job. I don't feel the need to consider these types.

dhysum · 2023-06-12T03:08:21Z

session/mock_bootstrap.go

+const (
+	defaultMockUpgradeToVerLatest = 0
+	// MockSimpleUpgradeToVerLatest is used to indicate the use of the simple mock bootstrapVersion.
+	MockSimpleUpgradeToVerLatest = 1


What does it mean 'Simple' here? The function 'mockSimpleUpgradeToVerLatest'?

And, the function and the enum are sharing the same name seems not a good idea.

Comments were added. Another name has a good suggestion can be put forward, I change it

ti-chi-bot · 2023-06-12T07:34:44Z

[LGTM Timeline notifier]

Timeline:

2023-06-09 05:11:21.661544004 +0000 UTC m=+61495.909465830: ☑️ agreed by Defined2014.
2023-06-12 07:34:43.487118343 +0000 UTC m=+137832.941879631: ☑️ agreed by hawkingrei.

ti-chi-bot · 2023-06-12T07:58:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Defined2014, hawkingrei, XuHuaiyu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Defined2014,XuHuaiyu,hawkingrei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2023-06-12T08:11:06Z

In response to a cherrypick label: new pull request created to branch release-7.1: #44582.

Signed-off-by: ti-chi-bot <[email protected]>

close #44158

ddl, session: fix re-upgrade issues

aee3cfd

zimulala requested a review from a team as a code owner June 7, 2023 04:01

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. needs-cherry-pick-release-7.1 Should cherry pick this PR to release-7.1 branch. labels Jun 7, 2023

ti-chi-bot bot requested a review from SeaRise June 7, 2023 04:01

ti-chi-bot bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 7, 2023

Merge branch 'master' into zimuxia/fix-reupgrade

0fec4eb

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 7, 2023

pingcap deleted a comment from ti-chi-bot bot Jun 7, 2023

zimulala requested a review from tangenta June 7, 2023 09:20

session: update bazel

35a2e85

Defined2014 reviewed Jun 9, 2023

View reviewed changes

session/mock_bootstrap.go Outdated Show resolved Hide resolved

dhysum reviewed Jun 9, 2023

View reviewed changes

session/bootstraptest/bootstrap_upgrade_test.go Show resolved Hide resolved

ddl, session: address comments and do tiny cleanup

e50bf5c

Defined2014 reviewed Jun 9, 2023

View reviewed changes

session/bootstraptest/bootstrap_upgrade_test.go Outdated Show resolved Hide resolved

session: address a comment

1291ed3

Defined2014 approved these changes Jun 9, 2023

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 9, 2023

dhysum reviewed Jun 12, 2023

View reviewed changes

session/bootstrap.go Show resolved Hide resolved

dhysum reviewed Jun 12, 2023

View reviewed changes

ddl/job_table.go Show resolved Hide resolved

dhysum reviewed Jun 12, 2023

View reviewed changes

session: add a comment

1e531f1

hawkingrei approved these changes Jun 12, 2023

View reviewed changes

ti-chi-bot bot added the lgtm label Jun 12, 2023

ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 12, 2023

XuHuaiyu approved these changes Jun 12, 2023

View reviewed changes

zimulala merged commit c18e60f into pingcap:master Jun 12, 2023

zimulala deleted the zimuxia/fix-reupgrade branch June 12, 2023 08:10

ti-chi-bot mentioned this pull request Jun 12, 2023

ddl, session: fix re-upgrade issues (#44469) #44582

Merged

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Jun 12, 2023

This is an automated cherry-pick of pingcap#44469

94727dc

Signed-off-by: ti-chi-bot <[email protected]>

ti-chi-bot bot pushed a commit that referenced this pull request Jul 12, 2023

ddl, session: fix re-upgrade issues (#44469) (#44582)

5af21fd

close #44158

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl, session: fix re-upgrade issues #44469

ddl, session: fix re-upgrade issues #44469

zimulala commented Jun 7, 2023 •

edited

Loading

Defined2014 left a comment

dhysum Jun 12, 2023

zimulala Jun 12, 2023

dhysum Jun 12, 2023 •

edited

Loading

zimulala Jun 12, 2023

dhysum Jun 12, 2023 •

edited

Loading

zimulala Jun 12, 2023

dhysum Jun 12, 2023

zimulala Jun 12, 2023

dhysum Jun 12, 2023

zimulala Jun 12, 2023

ti-chi-bot bot commented Jun 12, 2023

ti-chi-bot bot commented Jun 12, 2023

ti-chi-bot commented Jun 12, 2023

		}
		// TODO: consider about model.ActionRenameTables and model.ActionExchangeTablePartition, which need to get the schema names.

ddl, session: fix re-upgrade issues #44469

ddl, session: fix re-upgrade issues #44469

Conversation

zimulala commented Jun 7, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Defined2014 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhysum Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhysum Jun 12, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Jun 12, 2023

[LGTM Timeline notifier]

ti-chi-bot bot commented Jun 12, 2023

ti-chi-bot commented Jun 12, 2023

zimulala commented Jun 7, 2023 •

edited

Loading

dhysum Jun 12, 2023 •

edited

Loading

dhysum Jun 12, 2023 •

edited

Loading