checker(dm): change checker to more smart and gentle #3812

okJiang · 2021-12-09T10:27:41Z

What problem does this PR solve?

Issue Number: close #3766

What is changed and how it works?

tiflow/dm/docs/RFCS/20211130_enhanced_pre_checker.md

Lines 61 to 75 in 1fb41f9

    
           2. If task is full/all mode, the following items will be forced to check (correspondingly, it will not be check in increment mode): 
        
               - dump_privilege 
        
               - schema_of_shard_tables(only for sharding mode) 
        
           3. If task is increment/all mode, the following items will be forced to check (correspondingly, it will not be check in full mode): 
        
               - replication_privilege 
        
               - server_id 
        
               - binlog_enable 
        
               - binlog_format 
        
               - binlog_row_image 
        
               - online_ddl(new added) 
        
               - binlog_do_db(new added) 
        
           4. The following items will always be forced to check: 
        
               - version 
        
               - table_schema 
        
               - auto_increment_ID(only for sharding mode)

add binlog_do_db checker

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)

test case-sensitive

start MySQL with

[mysqld]
...
binlog_do_db = Upper_DB
binlog_do_db = Upper_DB1
lower_case_table_names = 1

mysql> show master status;
+------------------+----------+--------------------+------------------+-------------------------------------------+
| File             | Position | Binlog_Do_DB       | Binlog_Ignore_DB | Executed_Gtid_Set                         |
+------------------+----------+--------------------+------------------+-------------------------------------------+
| mysql-bin.000006 |     3249 | Upper_DB,Upper_DB1 |                  | a0ec2f1a-d811-11eb-84a7-16210329b3ba:1-52 |
+------------------+----------+--------------------+------------------+-------------------------------------------+

create table in upstream

create database `Upper_DB`;
create database `Upper_DB1`;
mysql> show databases;
+-------------------------+
| Database                |
+-------------------------+
| upper_db                |
| upper_db1               |
+-------------------------+

start dm-master/dm-worker and create source
check task with

case-sensitive: false

mysql-instances:
  - source-id: "mysql-replica-01"
    black-white-list:  "instance" # compatible with deprecated config
    mydumper-config-name: "global"
    loader-config-name: "global"
    syncer-config-name: "global"
    route-rules: ["global"]
    filter-rules: ["global"]

black-white-list: # compatible with deprecated config
  instance:
    do-dbs: ["Upper_DB*", "lower_db"]
    do-tables:
      - db-name: "Upper_DB*"
        tbl-name: "Do_Table*"
      - db-name: "Upper_DB"
        tbl-name: "lower_table*"
      - db-name: "lower_db"
        tbl-name: "Upper_Table*"

routes:
  global:
    schema-pattern: "Upper_DB"
    table-pattern: "Do_Table"
    target-schema: "UPPER_DB_ROUTE"
    target-table: "do_table_route"

check pass

test check failed(not in binlog_do_db)

If remove Upper_DB1 from binlog_do_db

mysql> show master status;
+------------------+----------+--------------+------------------+-------------------------------------------+
| File             | Position | Binlog_Do_DB | Binlog_Ignore_DB | Executed_Gtid_Set                         |
+------------------+----------+--------------+------------------+-------------------------------------------+
| mysql-bin.000007 |      194 | Upper_DB     |                  | a0ec2f1a-d811-11eb-84a7-16210329b3ba:1-52 |
+------------------+----------+--------------+------------------+-------------------------------------------+
1 row in set (0.00 sec)

check failed.

» check-task /home/xianjie/ticdc/dm/tests/case_sensitive/conf/dm-task.yaml
{
    "result": false,
    "msg": "[code=26005:class=dm-master:scope=internal:level=medium], Message: fail to check synchronization configuration with type: check was failed, please see detail
        detail: {
                "results": [
                        {
                                "id": 7,
                                "name": "binlog_do_db/binlog_ignore_db check",
                                "desc": "check whether dbs need replication is in binlog_do_db/binlog_ignore_db",
                                "state": "fail",
                                "extra": "these dbs [upper_db1] are not in binlog_do_db[upper_db]"
                        }
                ],
                "summary": {
                        "passed": false,
                        "total": 9,
                        "successful": 8,
                        "failed": 1,
                        "warning": 0
                }
        }"
}

Related changes

Need to update the documentation

Release note

Please add a release note.
If you don't think this PR needs a release note then fill it with `None`.

ti-chi-bot · 2021-12-09T10:27:42Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Ehco1996
lance6716

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.

okJiang · 2021-12-13T08:38:13Z

/run-verify
/run-dm-integration-test

okJiang · 2021-12-14T07:52:04Z

/run-dm-integration-test
/run-verify

okJiang · 2021-12-14T09:49:58Z

/run-dm-integration-test
/run-verify

okJiang · 2021-12-14T10:11:34Z

Due to sequence_sharding IT need ignore sharding table check, so the ci failed. Mainly because the task stop then start.

now upstream schema is conflict, ignore it and restart task

Wait the better sharding table check pr merge(not yet open).

okJiang · 2022-02-14T07:53:23Z

/run-dm-integration-test

…g/ticdc into pre-check-01 merge

ti-chi-bot · 2022-02-14T14:40:53Z

@okJiang: PR needs rebase.

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 kubernetes/test-infra repository.

codecov-commenter · 2022-02-16T11:00:45Z

Codecov Report

Merging #3812 (122b0b7) into master (9607554) will increase coverage by 0.0672%.
The diff coverage is 57.3113%.

Flag	Coverage Δ
cdc	`60.2329% <54.0983%> (+0.3107%)`	⬆️
dm	`51.8990% <58.8491%> (-0.1299%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master      #3812        +/-   ##
================================================
+ Coverage   55.6402%   55.7074%   +0.0672%     
================================================
  Files           494        507        +13     
  Lines         61283      63128      +1845     
================================================
+ Hits          34098      35167      +1069     
- Misses        23750      24469       +719     
- Partials       3435       3492        +57

dm/dm/config/checking_item.go

dm/pkg/utils/db.go

dm/pkg/checker/binlog.go

dm/pkg/utils/db.go

Ehco1996 · 2022-02-17T01:06:13Z

dm/pkg/checker/binlog.go

+}
+
+// NewBinlogDBChecker returns a RealChecker.
+func NewBinlogDBChecker(db *sql.DB, dbinfo *dbutil.DBConfig, schemas map[string]struct{}) RealChecker {


do we need an it test for this checker?

It is inconvenient to add it, because should set it in my.cnf.

Maybe manual test is enough.

manual test is updated in description.

Ehco1996 · 2022-02-17T01:10:05Z

dm/pkg/checker/binlog.go

+	// If both of them are empty, it will log changes for all DBs.
+	if len(binlogDoDB) != 0 {
+		for _, doDB := range binlogDoDBs {
+			delete(c.schemas, doDB)


where c.chemas init?

we also need to think about case-sensitive of this schemas

where c.chemas init?

tiflow/dm/checker/checker.go

Line 214 in 1e3dd15

checkSchemas := make(map[string]struct{}, len(mapping))

we also need to think about case-sensitive of this schemas

To be further confirmed. Because I think if schema name is case-sensitive, the binlog_do_db/binlog_ignore_db are too.

After test, binlog_db_db/binlog_ignore_db is not affected by lower_case_table_names.

[mysqld] binlog_do_db = Do binlog_do_db = TestCaseSensitive lower_case_table_names = 1 mysql> show master status; +------------------+----------+----------------------+------------------+--------------------------------------------+ | File | Position | Binlog_Do_DB | Binlog_Ignore_DB | Executed_Gtid_Set | +------------------+----------+----------------------+------------------+--------------------------------------------+ | mysql-bin.000004 | 194 | Do,TestCaseSensitive | | a0ec2f1a-d811-11eb-84a7-16210329b3ba:1-112 | +------------------+----------+----------------------+------------------+--------------------------------------------+ 1 row in set (0.00 sec)

I will add code about case-sensitive.

Co-authored-by: Ehco <[email protected]>

okJiang · 2022-02-18T06:56:26Z

/run-dm-integration-test

okJiang · 2022-02-18T09:37:21Z

/run-dm-integration-test

lance6716 · 2022-02-18T09:38:22Z

/check-issue-triage-complete

Ehco1996

LGTM great job

lance6716 · 2022-02-21T03:13:42Z

dm/checker/checker.go

@@ -259,7 +264,7 @@ func (c *Checker) Init(ctx context.Context) (err error) {
 					continue
 				}
 				if instance.cfg.ShardMode == config.ShardPessimistic {
-					c.checkList = append(c.checkList, checker.NewShardingTablesChecker(targetTableID, dbs, shardingSet, columnMapping, checkingShardID, dumpThreads))
+					c.checkList = append(c.checkList, checker.NewShardingTablesChecker(targetTableID, dbs, shardingSet, checkingShardID, dumpThreads))
 				} else {
 					c.checkList = append(c.checkList, checker.NewOptimisticShardingTablesChecker(targetTableID, dbs, shardingSet, dumpThreads))


we should also check AutoID for optimistic sharding mode. Will the optimistic table structure join itself can report the AutoID problem so we don't need extra checking logic?

yes, optimistic table structure join can report the autoID problem.

I will add some UT later

lance6716 · 2022-02-21T03:16:03Z

dm/dm/master/server.go

@@ -575,7 +575,7 @@ func (s *Server) StartTask(ctx context.Context, req *pb.StartTaskRequest) (*pb.S

 		resp.Result = true
 		if cfg.RemoveMeta {
-			resp.Msg += "`remove-meta` in task config is deprecated, please use `start-task ... --remove-meta` instead"
+			resp.Msg = "`remove-meta` in task config is deprecated, please use `start-task ... --remove-meta` instead"


please add a comment that what message will be overwritten

no message will be overwritten. The resp.Msg is just assigned once. Because this pr

msg, err := checker.CheckSyncConfigFunc(ctx, stCfgs, ctlcommon.DefaultErrorCnt, ctlcommon.DefaultWarnCnt) if err != nil { resp.CheckResult = terror.WithClass(err, terror.ClassDMMaster).Error() return resp, nil } resp.CheckResult = msg

resp.Msg is changed to resp.CheckResult.

So I just remove + from here.

lance6716 · 2022-02-21T03:31:03Z

dm/pkg/checker/binlog.go

+		binlogDoDB = strings.ToLower(binlogDoDB)
+		binlogIgnoreDB = strings.ToLower(binlogIgnoreDB)
+	}
+	binlogDoDB = strings.ReplaceAll(binlogDoDB, " ", "")


identifier can have spaces as long as they are backquoted, so this is incorrect

In fact, there is no spaces among the list of binlog_do_db. So, I just remove this later.

But another question, it seems that identifier also can have ,(comma)?

lance6716 · 2022-02-21T03:35:12Z

dm/pkg/checker/binlog.go

+		}
+		if len(c.schemas) > 0 {
+			dbs := []string{}
+			for db := range c.schemas {


we have a helper function SetToSlice

dm/pkg/checker/binlog.go

lance6716 · 2022-02-21T05:41:36Z

/merge

ti-chi-bot · 2022-02-21T05:41:38Z

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

Commit hash: f763630

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2021

okJiang mentioned this pull request Dec 28, 2021

checker(dm): adjust dump_privilege and version check #3894

Merged

save work

204cd63

okJiang force-pushed the pre-check-01 branch from dee5b2a to 204cd63 Compare February 14, 2022 07:50

Merge branch 'master' into pre-check-01

8abbe86

ti-chi-bot removed the do-not-merge/needs-linked-issue label Feb 14, 2022

okJiang added 3 commits February 14, 2022 17:33

add GetBinlogDb func

fd328f6

add binlog-db checker

368b563

Merge branches 'pre-check-01' and 'pre-check-01' of github.com:okJian…

24c5a30

…g/ticdc into pre-check-01 merge

ti-chi-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 14, 2022

okJiang marked this pull request as ready for review February 14, 2022 14:44

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2022

Merge branch 'master' of github.com:pingcap/tiflow into pre-check-01

73a5519

ti-chi-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 16, 2022

change test to testify

6dd727d

Ehco1996 reviewed Feb 17, 2022

View reviewed changes

okJiang and others added 5 commits February 18, 2022 10:17

Apply suggestions from code review

7616b85

Co-authored-by: Ehco <[email protected]>

address comment

ec4dca0

support case sensitive

37c77f5

fix

dae14a7

update desc

e229e83

okJiang added 2 commits February 18, 2022 16:50

fix verify

e18a907

add copy to checkSchemas(map type)

0176bbe

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

Ehco1996 approved these changes Feb 21, 2022

View reviewed changes

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

lance6716 reviewed Feb 21, 2022

View reviewed changes

okJiang added 3 commits February 21, 2022 13:23

add optimistic ut

e6d9afc

address comment

67c9155

address comment

f763630

lance6716 approved these changes Feb 21, 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 Feb 21, 2022

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

Merge branch 'master' into pre-check-01

122b0b7

ti-chi-bot merged commit 7f90eb5 into pingcap:master Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checker(dm): change checker to more smart and gentle #3812

checker(dm): change checker to more smart and gentle #3812

okJiang commented Dec 9, 2021 •

edited

Loading

ti-chi-bot commented Dec 9, 2021 •

edited

Loading

okJiang commented Dec 13, 2021

okJiang commented Dec 14, 2021

okJiang commented Dec 14, 2021

okJiang commented Dec 14, 2021 •

edited

Loading

okJiang commented Feb 14, 2022

ti-chi-bot commented Feb 14, 2022

codecov-commenter commented Feb 16, 2022 •

edited

Loading

Ehco1996 Feb 17, 2022

okJiang Feb 18, 2022

okJiang Feb 18, 2022

Ehco1996 Feb 17, 2022

Ehco1996 Feb 17, 2022

okJiang Feb 18, 2022

okJiang Feb 18, 2022 •

edited

Loading

okJiang commented Feb 18, 2022

okJiang commented Feb 18, 2022

lance6716 commented Feb 18, 2022

Ehco1996 left a comment

lance6716 Feb 21, 2022

okJiang Feb 21, 2022

lance6716 Feb 21, 2022

okJiang Feb 21, 2022

lance6716 Feb 21, 2022

okJiang Feb 21, 2022

lance6716 Feb 21, 2022

lance6716 commented Feb 21, 2022

ti-chi-bot commented Feb 21, 2022

	2. If task is full/all mode, the following items will be forced to check (correspondingly, it will not be check in increment mode):
	- dump_privilege
	- schema_of_shard_tables(only for sharding mode)
	3. If task is increment/all mode, the following items will be forced to check (correspondingly, it will not be check in full mode):
	- replication_privilege
	- server_id
	- binlog_enable
	- binlog_format
	- binlog_row_image
	- online_ddl(new added)
	- binlog_do_db(new added)
	4. The following items will always be forced to check:
	- version
	- table_schema
	- auto_increment_ID(only for sharding mode)

checker(dm): change checker to more smart and gentle #3812

checker(dm): change checker to more smart and gentle #3812

Conversation

okJiang commented Dec 9, 2021 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

test case-sensitive

test check failed(not in binlog_do_db)

Release note

ti-chi-bot commented Dec 9, 2021 • edited Loading

okJiang commented Dec 13, 2021

okJiang commented Dec 14, 2021

okJiang commented Dec 14, 2021

okJiang commented Dec 14, 2021 • edited Loading

okJiang commented Feb 14, 2022

ti-chi-bot commented Feb 14, 2022

codecov-commenter commented Feb 16, 2022 • edited Loading

Codecov Report

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

okJiang Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

okJiang commented Feb 18, 2022

okJiang commented Feb 18, 2022

lance6716 commented Feb 18, 2022

Ehco1996 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

lance6716 commented Feb 21, 2022

ti-chi-bot commented Feb 21, 2022

okJiang commented Dec 9, 2021 •

edited

Loading

ti-chi-bot commented Dec 9, 2021 •

edited

Loading

okJiang commented Dec 14, 2021 •

edited

Loading

codecov-commenter commented Feb 16, 2022 •

edited

Loading

okJiang Feb 18, 2022 •

edited

Loading