sync-diff-inspector: skip validation for tables that exist only upstream or downstream and print skipped information in summary and progress #693

liumengya94 · 2022-12-29T12:20:00Z

What problem does this PR solve?

Issue Number: ref #692

What is changed and how it works?

Add parameter skip-non-existing-table, which defaults to false.
Record and skip validation for tables that exist only upstream or downstream.
Show skipped information and row count comparison for missing tables in summary.txt.

Print progress information about skipped tables.

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change
Has exported variable/fields change

Side effects

None

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
Need to be included in the release note

ti-chi-bot · 2022-12-29T12:20:02Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

CLAassistant · 2022-12-29T12:20:06Z

All committers have signed the CLA.

liumengya94 · 2022-12-30T01:00:50Z

/run-all-tests

liumengya94 · 2022-12-30T01:27:37Z

/cc @lichunzhu

sync_diff_inspector/diff.go

sync_diff_inspector/report/report.go

sync_diff_inspector/source/source.go

sync_diff_inspector/progress/progress.go

tests/sync_diff_inspector/table_skip/run.sh

lichunzhu · 2023-01-05T08:29:38Z

sync_diff_inspector/source/source.go

+func AllTableExist(tableDiffs *common.TableDiff) bool {
+	if tableDiffs.NeedSkippedTable == common.AllTableExistFlag {
+		return true
+	}
+	return false
 }


Suggested change

func AllTableExist(tableDiffs *common.TableDiff) bool {

if tableDiffs.NeedSkippedTable == common.AllTableExistFlag {

return true

}

return false

}

func AllTableExist(needSkipTable int) bool {

return tableDiffs.NeedSkippedTable == common.AllTableExistFlag

}

lichunzhu · 2023-01-05T08:32:57Z

sync_diff_inspector/diff.go

+		var isEqual, isSkip bool
+		isAllTableExist := tables[tableIndex].NeedSkippedTable
+		if source.AllTableExist(tables[tableIndex]) {
+			var err error
+			isEqual, isSkip, err = df.compareStruct(ctx, tableIndex)
+			if err != nil {
+				return errors.Trace(err)
+			}
+		} else {
+			isEqual, isSkip = false, true
 		}


Suggested change

var isEqual, isSkip bool

isAllTableExist := tables[tableIndex].NeedSkippedTable

if source.AllTableExist(tables[tableIndex]) {

var err error

isEqual, isSkip, err = df.compareStruct(ctx, tableIndex)

if err != nil {

return errors.Trace(err)

}

} else {

isEqual, isSkip = false, true

}

isEqual, isSkip, isAllTableExist := false, true, tables[tableIndex].NeedSkippedTable

if source.AllTableExist(tables[tableIndex]) {

var err error

isEqual, isSkip, err = df.compareStruct(ctx, tableIndex)

if err != nil {

return errors.Trace(err)

}

}

lichunzhu · 2023-01-05T09:12:01Z

sync_diff_inspector/progress/progress_test.go

@@ -24,12 +24,12 @@ import (

 func TestProgress(t *testing.T) {
 	p := NewTableProgressPrinter(4, 0)
-	p.RegisterTable("1", true, true)
+	p.RegisterTable("1", true, true, 1)


Use constant instead of 1 0.

lichunzhu · 2023-01-05T09:13:06Z

sync_diff_inspector/report/report.go

+	ChunkMap     map[string]*ChunkResult `json:"chunk-result"` // `ChunkMap` stores the `ChunkResult` of each chunk of the table
+	UpCount      int64                   `json:"up-count"`     // `UpCount` is the number of rows in the table from upstream
+	DownCount    int64                   `json:"down-count"`   // `DownCount` is the number of rows in the table from downstream
+	TableSkipped int                     `json:"table-skipped"`


Maybe TableLack to differ from DataSkip

sync_diff_inspector/report/report.go

lichunzhu · 2023-01-05T12:15:22Z

sync_diff_inspector/report/report_test.go

@@ -104,7 +110,7 @@ func TestReport(t *testing.T) {
 	report.CalculateTotalSize(ctx, db)

 	// Test Table Report
-	report.SetTableStructCheckResult("test", "tbl", true, false)
+	report.SetTableStructCheckResult("test", "tbl", true, false, 0)


tests/sync_diff_inspector/table_skip/run.sh

lichunzhu

Rest LGTM

lichunzhu · 2023-01-06T09:43:56Z

sync_diff_inspector/diff.go

 	if rangeInfo.ChunkRange.Type == chunk.Empty {
 		dml.node.State = checkpoints.IgnoreState
+		// for tables that don't exist upstream or downstream
+		if !source.AllTableExist(tableDiff) {
+			upCount, _ := dbutil.GetRowCount(ctx, df.upstream.GetDB(), schema, table, "", nil)


What if upstream has multiple mysql sources?

lichunzhu · 2023-01-06T10:12:18Z

sync_diff_inspector/progress/progress.go

+			if isExist == common.UpstreamTableLackFlag {
+				state = TABLE_STATE_NOT_EXSIT_UPSTREAM | TABLE_STATE_REGISTER
+			} else if isExist == common.DownstreamTableLackFlag {
+				state = TABLE_STATE_NOT_EXSIT_DOWNSTREAM | TABLE_STATE_REGISTER
+			} else {
+				state = TABLE_STATE_RESULT_FAIL_STRUCTURE_DONE | TABLE_STATE_REGISTER
+			}


Suggested change

if isExist == common.UpstreamTableLackFlag {

state = TABLE_STATE_NOT_EXSIT_UPSTREAM | TABLE_STATE_REGISTER

} else if isExist == common.DownstreamTableLackFlag {

state = TABLE_STATE_NOT_EXSIT_DOWNSTREAM | TABLE_STATE_REGISTER

} else {

state = TABLE_STATE_RESULT_FAIL_STRUCTURE_DONE | TABLE_STATE_REGISTER

}

switch isExist {

case common.UpstreamTableLackFlag:

state = TABLE_STATE_NOT_EXSIT_UPSTREAM | TABLE_STATE_REGISTER

case common.DownstreamTableLackFlag:

state = TABLE_STATE_NOT_EXSIT_DOWNSTREAM | TABLE_STATE_REGISTER

default:

state = TABLE_STATE_RESULT_FAIL_STRUCTURE_DONE | TABLE_STATE_REGISTER

}

lichunzhu · 2023-01-29T07:07:39Z

sync_diff_inspector/diff.go

-		isEqual, isSkip, isAllTableExist := false, true, tables[tableIndex].NeedSkippedTable
-		if source.AllTableExist(tables[tableIndex]) {
+		isEqual, isSkip, isAllTableExist := false, true, tables[tableIndex].TableLack
+		if common.AllTableExist(tables[tableIndex].TableLack) {


Suggested change

if common.AllTableExist(tables[tableIndex].TableLack) {

if common.AllTableExist(isAllTableExist) {

lichunzhu · 2023-01-29T07:12:27Z

sync_diff_inspector/diff.go

+			upCount := df.upstream.GetCountAndCrc32(ctx, rangeInfo).Count
+			downCount := df.downstream.GetCountAndCrc32(ctx, rangeInfo).Count


We only need count here. However, this function will also compute checksum. I'm afraid this will affect the efficiency.

Got it. I defined a new interface GetCountForLackTable in the next commit(34293bb).

lichunzhu · 2023-01-29T07:17:26Z

sync_diff_inspector/source/mysql_shard.go

+		var count int64
+		if matchSources != nil {
+			for _, ms := range matchSources {
+				count, err = dbutil.GetRowCount(ctx, ms.DBConn, ms.OriginSchema, ms.OriginTable, "", nil)


This error will be returned in ChecksumInfo. Is that expected?

lichunzhu · 2023-01-29T07:26:45Z

sync_diff_inspector/source/mysql_shard.go

+				var isMatched bool
 				// get all tables from all source db instance
 				if f.MatchTable(targetSchema, targetTable) {


Suggested change

var isMatched bool

// get all tables from all source db instance

if f.MatchTable(targetSchema, targetTable) {

isMatched := f.MatchTable(targetSchema, targetTable)

if isMatched {

lichunzhu · 2023-01-29T07:27:29Z

sync_diff_inspector/source/tidb.go

+			var isMatched bool
 			if f.MatchTable(targetSchema, targetTable) {
 				// if match the filter, we should respect it and check target has this table later.
 				sourceTablesAfterRoute[uniqueId] = struct{}{}
+				isMatched = true
 			}


lichunzhu

LGTM

lichunzhu · 2023-01-30T10:10:50Z

/merge

ti-chi-bot · 2023-01-30T10:10:54Z

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

Commit hash: 8d7c343

ti-chi-bot · 2023-01-30T10:11:07Z

@liumengya94: Your PR was out of date, I have automatically updated it for you.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

liumengya94 · 2023-01-30T11:56:39Z

/run-unit-test

print information about the skipped table in summary

aa34581

ti-chi-bot added the first-time-contributor label Dec 29, 2022

ti-chi-bot requested review from 3pointer and kennytm December 29, 2022 12:20

ti-chi-bot added the size/XL label Dec 29, 2022

liumengya94 added 2 commits December 29, 2022 21:29

print information about the skipped table in summary

1e47104

print information about the skipped table in summary

d8c7f6d

ti-chi-bot requested a review from lichunzhu December 30, 2022 01:27

add parameter skip-non-existing-table

72a2bb6

lichunzhu reviewed Jan 3, 2023

View reviewed changes

sync_diff_inspector/diff.go Outdated Show resolved Hide resolved

sync_diff_inspector/diff.go Outdated Show resolved Hide resolved

sync_diff_inspector/report/report.go Outdated Show resolved Hide resolved

sync_diff_inspector/source/source.go Outdated Show resolved Hide resolved

print skipped information in progress

5b586f6

lichunzhu reviewed Jan 4, 2023

View reviewed changes

sync_diff_inspector/progress/progress.go Outdated Show resolved Hide resolved

sync_diff_inspector/progress/progress.go Outdated Show resolved Hide resolved

tests/sync_diff_inspector/table_skip/run.sh Show resolved Hide resolved

liumengya94 added 3 commits January 4, 2023 20:25

fix some error

54dc8df

fix some error

f701426

update test

5a42342

update

54e2ab9

ti-chi-bot added size/XXL and removed size/XL labels Jan 5, 2023

update test

8a0f59f

lichunzhu reviewed Jan 5, 2023

View reviewed changes

update test

4736864

ti-chi-bot added size/XL and removed size/XXL labels Jan 6, 2023

lichunzhu reviewed Jan 6, 2023

View reviewed changes

fix

3f254c8

fix code

5af05c2

ti-chi-bot added size/XXL and removed size/XL labels Jan 18, 2023

liumengya94 added 2 commits January 19, 2023 20:52

fix

b928386

fix

a8b82a9

liumengya94 mentioned this pull request Jan 19, 2023

sync-diff: add description for the new parameter skip-non-existing-table pingcap/docs-cn#12776

Merged

15 tasks

lichunzhu reviewed Jan 29, 2023

View reviewed changes

liumengya94 added 5 commits January 29, 2023 17:29

add function GetCountForLackTable

34293bb

fix test

03b86f3

fix test

8581b7b

fix test

a637154

fix test

8d7c343

lichunzhu approved these changes Jan 30, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 label Jan 30, 2023

ti-chi-bot added the status/can-merge label Jan 30, 2023

Merge branch 'master' into fix-summary

4d78f73

ti-chi-bot merged commit b4be17a into pingcap:master Jan 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sync-diff-inspector: skip validation for tables that exist only upstream or downstream and print skipped information in summary and progress #693

sync-diff-inspector: skip validation for tables that exist only upstream or downstream and print skipped information in summary and progress #693

liumengya94 commented Dec 29, 2022 •

edited

Loading

ti-chi-bot commented Dec 29, 2022 •

edited

Loading

CLAassistant commented Dec 29, 2022 •

edited

Loading

liumengya94 commented Dec 30, 2022

liumengya94 commented Dec 30, 2022

lichunzhu Jan 5, 2023

lichunzhu Jan 5, 2023

lichunzhu Jan 5, 2023

lichunzhu Jan 5, 2023

lichunzhu Jan 5, 2023

lichunzhu left a comment

lichunzhu Jan 6, 2023

lichunzhu Jan 6, 2023

lichunzhu Jan 29, 2023

lichunzhu Jan 29, 2023

liumengya94 Jan 29, 2023

lichunzhu Jan 29, 2023

lichunzhu Jan 29, 2023

lichunzhu Jan 29, 2023

liumengya94 Jan 29, 2023

lichunzhu left a comment

lichunzhu commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

liumengya94 commented Jan 30, 2023

	if common.AllTableExist(tables[tableIndex].TableLack) {
	if common.AllTableExist(isAllTableExist) {

		upCount := df.upstream.GetCountAndCrc32(ctx, rangeInfo).Count
		downCount := df.downstream.GetCountAndCrc32(ctx, rangeInfo).Count

sync-diff-inspector: skip validation for tables that exist only upstream or downstream and print skipped information in summary and progress #693

sync-diff-inspector: skip validation for tables that exist only upstream or downstream and print skipped information in summary and progress #693

Conversation

liumengya94 commented Dec 29, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

ti-chi-bot commented Dec 29, 2022 • edited Loading

CLAassistant commented Dec 29, 2022 • edited Loading

liumengya94 commented Dec 30, 2022

liumengya94 commented Dec 30, 2022

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

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

lichunzhu left a comment

Choose a reason for hiding this comment

lichunzhu commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

ti-chi-bot commented Jan 30, 2023

liumengya94 commented Jan 30, 2023

liumengya94 commented Dec 29, 2022 •

edited

Loading

ti-chi-bot commented Dec 29, 2022 •

edited

Loading

CLAassistant commented Dec 29, 2022 •

edited

Loading