binlog: do not decode rows for block table #7622

GMHDBJD · 2022-11-16T09:26:44Z

What problem does this PR solve?

Issue Number: ref #4287
pingcap/dm#924 pingcap/dm#2281 go-mysql-org/go-mysql#737

What is changed and how it works?

do not decode rows for block table

Check List

Tests

Unit test
Integration test

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

do not decode rows for block table

ti-chi-bot · 2022-11-16T09:26:45Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

buchuitoudegou
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.

GMHDBJD · 2022-11-17T05:57:20Z

/run-all-tests

codecov-commenter · 2022-11-17T06:45:51Z

Codecov Report

Merging #7622 (71ce943) into master (0438dd4) will decrease coverage by 0.2552%.
The diff coverage is 36.2754%.

Additional details and impacted files

Flag	Coverage Δ
cdc	`66.2273% <34.7921%> (-0.6895%)`	⬇️
dm	`52.1216% <21.5686%> (+0.0388%)`	⬆️
engine	`63.7677% <68.1818%> (-0.0183%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #7622        +/-   ##
================================================
- Coverage   60.0458%   59.7906%   -0.2553%     
================================================
  Files           807        811         +4     
  Lines         92391      92869       +478     
================================================
+ Hits          55477      55527        +50     
- Misses        32099      32497       +398     
- Partials       4815       4845        +30

buchuitoudegou · 2022-11-17T10:28:08Z

dm/syncer/binlogstream/streamer_controller.go

@@ -288,7 +288,7 @@ func (c *StreamerController) resetReplicationSyncer(tctx *tcontext.Context, loca
 	if c.currentBinlogType == RemoteBinlog {
 		c.streamProducer = &remoteBinlogReader{replication.NewBinlogSyncer(c.syncCfg), tctx, c.syncCfg.Flavor, c.enableGTID}


Do we need to abandon these blocked events while using a remote binlog reader?

c.syncCfg is created by

tiflow/dm/syncer/util.go

Line 142 in d060d48

func subtaskCfg2BinlogSyncerCfg(cfg *config.SubTaskConfig, timezone *time.Location, baList *filter.Filter) (replication.BinlogSyncerConfig, error) {

, which abandon these blocked events

buchuitoudegou · 2022-11-17T10:30:45Z

dm/syncer/util.go

+			}
+			if skipByTable(baList, tb) {
+				if len(blockListCache) >= maxCapacity {
+					blockListCache = make(map[uint64]struct{}, maxCapacity)


I will prefer to evict one record rather than reset the whole cache. Maybe an LRU or CLOCK cache is more appropriate.

But it might be rare to run out of capacity, so it's okay.

add a TODO c5511e7

buchuitoudegou · 2022-11-17T10:32:48Z

dm/tests/binlog_parse/run.sh

+source $cur/../_utils/test_prepare
+WORK_DIR=$TEST_DIR/$TEST_NAME
+
+function run() {


would you leave a comment to illustrate what this test intends to do?

do you mean this used to manually test MariaDB? I remember we don't have MariaDB in CI

buchuitoudegou · 2022-11-17T10:33:40Z

go.mod

@@ -26,7 +26,7 @@ require (
 	github.com/gin-gonic/gin v1.7.4
 	github.com/glebarez/go-sqlite v1.17.3
 	github.com/glebarez/sqlite v1.4.6
-	github.com/go-mysql-org/go-mysql v1.6.1-0.20220718092400-c855c26b37bd
+	github.com/go-mysql-org/go-mysql v1.6.1-0.20221116091419-49d58c4c3e4c


would be grateful if you comment on the code to specify the change in this repo.

add link in pr header

GMHDBJD · 2022-11-18T06:05:17Z

/run-all-tests

GMHDBJD · 2022-11-22T04:36:05Z

/run-all-tests

GMHDBJD · 2022-11-22T06:41:06Z

/run-all-tests

lance6716

14 / 19 files viewed

lance6716 · 2022-11-23T02:31:52Z

dm/tests/others_integration_1.txt

@@ -9,3 +9,5 @@ sequence_sharding_optimistic
 sequence_sharding_removemeta
 gtid
 only_dml
+adjust_gtid


it's moved to the first line

lance6716 · 2022-11-23T02:33:39Z

dm/tests/binlog_parse/run.sh

+source $cur/../_utils/test_prepare
+WORK_DIR=$TEST_DIR/$TEST_NAME
+
+function run() {


do you mean this used to manually test MariaDB? I remember we don't have MariaDB in CI

lance6716

rest lgtm

lance6716 · 2022-11-23T10:08:30Z

dm/tests/others_integration_1.txt

@@ -9,3 +9,4 @@ sequence_sharding_optimistic
 sequence_sharding_removemeta
 gtid
 only_dml
+binlog_parse


others_integration_1.txt is the slowest stage in CI! please move it to other_2 or 3

GMHDBJD · 2022-11-24T02:47:45Z

/merge

ti-chi-bot · 2022-11-24T02:47:48Z

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

Commit hash: 9a3e548

ti-chi-bot · 2022-11-24T02:48:01Z

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

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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.

binlog: do not decode rows for blaock table

eccd29a

ti-chi-bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Nov 16, 2022

GMHDBJD added status/ptal Could you please take a look? area/dm Issues or PRs related to DM. labels Nov 16, 2022

ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 16, 2022

Merge branch 'master' into filterBinlog

d060d48

GMHDBJD requested review from D3Hunter, lance6716 and buchuitoudegou November 17, 2022 05:57

buchuitoudegou reviewed Nov 17, 2022

View reviewed changes

address comment

c5511e7

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

buchuitoudegou approved these changes Nov 18, 2022

View reviewed changes

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

Merge branch 'master' into filterBinlog

faa5d80

fix it

8d98b89

fix it

71ce943

lance6716 reviewed Nov 23, 2022

View reviewed changes

address comment

aed1214

lance6716 approved these changes Nov 23, 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 Nov 23, 2022

address comment

9a3e548

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

Merge branch 'master' into filterBinlog

a52bbba

ti-chi-bot merged commit 9db6370 into pingcap:master Nov 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

binlog: do not decode rows for block table #7622

binlog: do not decode rows for block table #7622

GMHDBJD commented Nov 16, 2022 •

edited

Loading

ti-chi-bot commented Nov 16, 2022 •

edited

Loading

GMHDBJD commented Nov 17, 2022

codecov-commenter commented Nov 17, 2022 •

edited

Loading

buchuitoudegou Nov 17, 2022

GMHDBJD Nov 17, 2022

buchuitoudegou Nov 17, 2022

GMHDBJD Nov 17, 2022

buchuitoudegou Nov 17, 2022

GMHDBJD Nov 17, 2022

lance6716 Nov 23, 2022

GMHDBJD Nov 23, 2022 •

edited

Loading

buchuitoudegou Nov 17, 2022

GMHDBJD Nov 17, 2022

GMHDBJD commented Nov 18, 2022

GMHDBJD commented Nov 22, 2022

GMHDBJD commented Nov 22, 2022

lance6716 left a comment

lance6716 Nov 23, 2022

lance6716 Nov 23, 2022

lance6716 left a comment

lance6716 Nov 23, 2022

GMHDBJD commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

		@@ -288,7 +288,7 @@ func (c StreamerController) resetReplicationSyncer(tctx tcontext.Context, loca
		if c.currentBinlogType == RemoteBinlog {
		c.streamProducer = &remoteBinlogReader{replication.NewBinlogSyncer(c.syncCfg), tctx, c.syncCfg.Flavor, c.enableGTID}

binlog: do not decode rows for block table #7622

binlog: do not decode rows for block table #7622

Conversation

GMHDBJD commented Nov 16, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Tests

Questions

Will it cause performance regression or break compatibility?

Do you need to update user documentation, design documentation or monitoring documentation?

Release note

ti-chi-bot commented Nov 16, 2022 • edited Loading

GMHDBJD commented Nov 17, 2022

codecov-commenter commented Nov 17, 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

Choose a reason for hiding this comment

GMHDBJD Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD commented Nov 18, 2022

GMHDBJD commented Nov 22, 2022

GMHDBJD commented Nov 22, 2022

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

lance6716 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

ti-chi-bot commented Nov 24, 2022

GMHDBJD commented Nov 16, 2022 •

edited

Loading

ti-chi-bot commented Nov 16, 2022 •

edited

Loading

codecov-commenter commented Nov 17, 2022 •

edited

Loading

GMHDBJD Nov 23, 2022 •

edited

Loading