bug-fix: use table-info-before always and fix bug for recover lock in optimistic #1518

GMHDBJD · 2021-03-18T12:34:14Z

What problem does this PR solve?

#1510 case4

What is changed and how it works?

if table-info-before of an info is different with table saved in master(like worker restart and TrySync twice)
- use table-info-before always
- joined tables base on table-info-before as oldJoined
when recovering lock
- join all info
- build lock with joined info, update tables with tableInfoBefore
- sort the infos, TrySync them one by one

Check List

Tests

unit test
integration test

lichunzhu · 2021-03-19T09:37:25Z

Is this PR still WIP?

lichunzhu · 2021-03-19T09:56:17Z

pkg/shardddl/optimism/lock.go

@@ -72,7 +72,7 @@ func NewLock(ID, task, downSchema, downTable string, ti *model.TableInfo, tts []
 		synced:     true,
 		versions:   make(map[string]map[string]map[string]int64),
 	}
-	l.addTables(tts)
+	l.addTables(l.joined, tts)


What if tts contains the other tables whose schema didn't reach l.joined?

Here l.joined equals tableInfoBefore. If it's a new lock(first table), tableInfoBefore=l.joined. For other newer tables, they will be added in L163

The tts here is from https://github.com/pingcap/dm/blob/master/dm/master/shardddl/optimist.go#L267. optimistic will get all source tables from etcd and this tts may have not only the table in this info, I think.

em... That's right, I will fix it

pkg/terror/error_list.go

pkg/shardddl/optimism/info.go

dm/master/shardddl/optimist_test.go

lance6716 · 2021-03-23T02:23:44Z

pkg/shardddl/optimism/lock.go

 	// handle the case where <callerSource, callerSchema, callerTable>
 	// is not in old source tables and current new source tables.
 	// duplicate append is not a problem.
 	tts = append(tts, newTargetTable(l.Task, callerSource, l.DownSchema, l.DownTable,
 		map[string]map[string]struct{}{callerSchema: {callerTable: struct{}{}}}))
 	// add any new source tables.
 	l.addTables(tts)
+	l.receiveTable(callerSource, callerSchema, callerTable, oldTable)
 	if val, ok := l.versions[callerSource][callerSchema][callerTable]; !ok || val < infoVersion {
 		l.versions[callerSource][callerSchema][callerTable] = infoVersion
 	}


now we changed the l.tables by l.receiveTable, so in line 179 oldJoined is not a result of "join every l.tables" 🤔

Did you mean join all the tables for old joined?

I'm just not sure if the correctness depends on "old joined should be joined result of all l.tables", or "old joined of previous TrySync and new joined of next TrySync should keep consistent"

If we assume user start-task with same table info in all upstream, tableInfoBefore should be same as oldJoined or oldtable. Otherwise, the oldJoined is not collect and we may keep it and hope later join may return an error? cc @lichunzhu

If dm-master/dm-worker meets a restart the table info in all upstream is more likely to be different. What's the problem here? I don't understand this clearly.

But if the tableInfoBefore not equal l.tables[source][schema][table], that means the l.tables[source][schema][table] and oldjoined may wrong, do we need join all the tables with tableInfoBefore as oldjoined?

But if the tableInfoBefore not equal l.tables[source][schema][table], that means the l.tables[source][schema][table] and oldjoined may wrong, do we need join all the tables with tableInfoBefore as oldjoined?

I think carefully about this situation. Could you help me check if I'm wrong?

When this happens, it means we add a table whose table schema is not equal to joined table info now. When we init all the tables, we have three situations:

This task is new. The pre-check will assure the correctness of the tables' schemas.

This dm cluster recovers from a failure. The lock is synced now and all the tables have the same schema. If we add a table with a different schema, we should report an error.

This dm cluster recovers from a failure. The lock is unsynced now. Some workers didn't put their lock Info into etcd before the restart. These tables' l.tables will be set to l.joined at first, and if it's different from the tableInfoBefore received later I think it's acceptable. Am I right?

So you mean if we get a info which we have already received the table info before, if their schema is different, we should report an error?

Yes. But I don't think we should report an error in situation 3.

Some workers didn't put their lock Info into etcd before the restart. These tables' l.tables will be set to l.joined at first

That means the table haven't been received. l.received[x][x][x]==false

In unit test, we have some idempotent TrySync, how do we deal with the case? 🤔

Co-authored-by: lance6716 <lance6716@gmail.com>

lichunzhu · 2021-03-25T06:39:17Z

tests/shardddl1/run.sh

+#    start=1
+#    end=35
+#    except=(024 025 029)
+#    for i in $(seq -f "%03g" ${start} ${end}); do
+#        if [[ ${except[@]} =~ $i ]]; then
+#            continue
+#        fi
+#        DM_${i}
+#        sleep 1
+#    done
+#    DM_RENAME_COLUMN_OPTIMISTIC


lance6716

will review later

lance6716 · 2021-03-25T07:02:32Z

dm/master/shardddl/optimist.go

+						newJoined, err := joined.Join(schemacmp.Encode(info.TableInfoBefore))
+						// ignore error, will report it in TrySync later
+						if err != nil {
+							o.logger.Error(fmt.Sprintf("fail to join table info %s with %s, lockID: %s in recover lock", joined, newJoined, lockID), log.ShortError(err))


we could add a alerting rule for errors of Optimist initialization in future

lance6716 · 2021-03-25T07:13:52Z

dm/master/shardddl/optimist_test.go

+	infos = sortInfos(ifm)
+	c.Assert(len(infos), Equals, 3)
+
+	i11.Version = 2


seems we could replace Info.Version by Revision (could file another PR)

lance6716 · 2021-03-25T07:23:52Z

pkg/shardddl/optimism/keeper_test.go

+		i11 = NewInfo(task, source1, upSchema, upTable, downSchema, downTable, DDLs1, ti0, []*model.TableInfo{ti1})
+		i21 = NewInfo(task, source2, upSchema, upTable, downSchema, downTable, DDLs2, ti2, []*model.TableInfo{ti3})
+
+		tts = []TargetTable{


"target" in TargetTable makes me think of "source/target", where the "target" means downstream tables. We could rename it to "TableMap" or something in future

lance6716 · 2021-03-25T07:28:14Z

pkg/shardddl/optimism/lock_test.go

@@ -1580,3 +1584,81 @@ func newInfoWithVersion(task, source, upSchema, upTable, downSchema, downTable s
 	info.Version = vers[source][upSchema][upTable]
 	return info
 }
+
+func (t *testLock) TestLockTrySyncReceived(c *C) {


which case does it test?

rename in 814d7a7 . Now we use table-info-before for all table, create a different index will not be in conflict now.

tests/shardddl1/run.sh

Co-authored-by: lance6716 <lance6716@gmail.com>

lichunzhu

Rest LGTM

lichunzhu · 2021-03-25T10:01:18Z

tests/shardddl1/run.sh

+    run_sql_source2 "insert into ${shardddl1}.${tb2} values(7,7);"
+    check_log_contain_with_retry "putted a shard DDL.*tb2.*ALTER TABLE .* ADD COLUMN" $WORK_DIR/worker1/log/dm-worker.log $WORK_DIR/worker2/log/dm-worker.log 
+
+    # recover lock, tb1's info: (a,b,c)->(a,c); tb2's info: (a)->(a,b)


Do we cover case#4 at this place?

I think yes? Though the order cannot determine, but we don't depend on the order now

lichunzhu · 2021-03-25T11:33:43Z

/lgtm

lance6716 · 2021-03-26T01:59:15Z

/lgtm

ti-chi-bot · 2021-03-26T01:59:19Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

lance6716
lichunzhu

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

lance6716 · 2021-03-26T01:59:30Z

/run-compatibility-tests

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot · 2021-03-26T02:57:12Z

cherry pick to release-2.0 in PR #1536

… optimistic (#1518) (#1536)

use tableInfoBefore instead of joined table

0dfad92

fix ut

5101a9d

Merge branch 'master' into useTableInfoBefore

60f5f43

GMHDBJD added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Mar 19, 2021

lichunzhu reviewed Mar 19, 2021

View reviewed changes

GMHDBJD added status/WIP This PR is still work in progress and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Mar 19, 2021

GMHDBJD added 4 commits March 22, 2021 15:17

add received

08fe90f

sort info by revision when recover

21d75d9

add ut

0325de4

add more ut

1cd95d0

lance6716 reviewed Mar 23, 2021

View reviewed changes

GMHDBJD and others added 7 commits March 23, 2021 16:19

join all infos and use table-info-before as table info

0a67c32

Update pkg/terror/error_list.go

f9153cd

Co-authored-by: lance6716 <lance6716@gmail.com>

rename revision

cb236a0

rename test func

bebd18b

update errors

0196510

remove debug

65bd0af

Merge branch 'master' into useTableInfoBefore

2f5e68f

ti-chi-bot added the size/XXL label Mar 23, 2021

lance6716 added this to the v2.0.2 milestone Mar 24, 2021

GMHDBJD added 3 commits March 25, 2021 10:32

join all infos based on table-info-before if different

ff42ec6

Merge remote-tracking branch 'upstream/master' into useTableInfoBefore

5e04e8f

fix integration test

c660ba9

lichunzhu reviewed Mar 25, 2021

View reviewed changes

revert debug

8450ef4

GMHDBJD changed the title ~~optimistic: use tableInfoBefore instead of joined table if table not exist~~ bug-fix: use table-info-before always and fix bug for recover lock in optimistic Mar 25, 2021

lance6716 reviewed Mar 25, 2021

View reviewed changes

GMHDBJD and others added 3 commits March 25, 2021 17:33

address comment

814d7a7

Update tests/shardddl1/run.sh

fb89276

Co-authored-by: lance6716 <lance6716@gmail.com>

Merge branch 'master' into useTableInfoBefore

d274e6b

lichunzhu reviewed Mar 25, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Mar 25, 2021

Merge branch 'master' into useTableInfoBefore

fadfd2b

ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Mar 26, 2021

GMHDBJD merged commit 69f65ca into pingcap:master Mar 26, 2021

GMHDBJD deleted the useTableInfoBefore branch March 26, 2021 02:56

ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Mar 26, 2021

cherry pick pingcap#1518 to release-2.0

c614263

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>

ti-srebot mentioned this pull request Mar 26, 2021

bug-fix: use table-info-before always and fix bug for recover lock in optimistic (#1518) #1536

Merged

ti-chi-bot pushed a commit that referenced this pull request Mar 26, 2021

bug-fix: use table-info-before always and fix bug for recover lock in…

ef4a6f1

… optimistic (#1518) (#1536)

sleepymole mentioned this pull request Apr 1, 2021

shardddl/optimistic: use init-schema for table without info when recover lock #1551

Merged

ti-srebot mentioned this pull request Apr 3, 2021

shardddl/optimistic: use init-schema for table without info when recover lock (#1551) #1553

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug-fix: use table-info-before always and fix bug for recover lock in optimistic #1518

bug-fix: use table-info-before always and fix bug for recover lock in optimistic #1518

GMHDBJD commented Mar 18, 2021 •

edited

Loading

lichunzhu commented Mar 19, 2021

lichunzhu Mar 19, 2021

GMHDBJD Mar 19, 2021

lichunzhu Mar 19, 2021

GMHDBJD Mar 19, 2021

lance6716 Mar 23, 2021

GMHDBJD Mar 23, 2021

lance6716 Mar 23, 2021 •

edited

Loading

GMHDBJD Mar 23, 2021 •

edited

Loading

lichunzhu Mar 24, 2021

GMHDBJD Mar 24, 2021

lichunzhu Mar 24, 2021

GMHDBJD Mar 24, 2021 •

edited

Loading

lichunzhu Mar 24, 2021 •

edited

Loading

GMHDBJD Mar 24, 2021 •

edited

Loading

lichunzhu Mar 25, 2021

lance6716 left a comment

lance6716 Mar 25, 2021

lance6716 Mar 25, 2021

lance6716 Mar 25, 2021

lance6716 Mar 25, 2021

GMHDBJD Mar 25, 2021

lichunzhu left a comment

lichunzhu Mar 25, 2021

GMHDBJD Mar 25, 2021

lichunzhu commented Mar 25, 2021

lance6716 commented Mar 26, 2021

ti-chi-bot commented Mar 26, 2021

lance6716 commented Mar 26, 2021

ti-srebot commented Mar 26, 2021

bug-fix: use table-info-before always and fix bug for recover lock in optimistic #1518

bug-fix: use table-info-before always and fix bug for recover lock in optimistic #1518

Conversation

GMHDBJD commented Mar 18, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

lichunzhu commented Mar 19, 2021

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

lance6716 Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

GMHDBJD Mar 23, 2021 • 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

GMHDBJD Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

lichunzhu Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

GMHDBJD Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 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

lichunzhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lichunzhu commented Mar 25, 2021

lance6716 commented Mar 26, 2021

ti-chi-bot commented Mar 26, 2021

lance6716 commented Mar 26, 2021

ti-srebot commented Mar 26, 2021

GMHDBJD commented Mar 18, 2021 •

edited

Loading

lance6716 Mar 23, 2021 •

edited

Loading

GMHDBJD Mar 23, 2021 •

edited

Loading

GMHDBJD Mar 24, 2021 •

edited

Loading

lichunzhu Mar 24, 2021 •

edited

Loading

GMHDBJD Mar 24, 2021 •

edited

Loading