pkg(both): a separate library for DML row change #4376

lance6716 · 2022-01-18T04:16:55Z

What problem does this PR solve?

Issue Number: ref #3242

What is changed and how it works?

provide a representation for DML (row change), and some conversion of it. Also fix some wrong behaviour of original code (such as we should decide generated column from target table structure)

next PR I'll let DM adopt this library, and future plans are

CDC adopt this library
propose a DML worker which include reduce/causality/executeSQL functionality

Check List

Tests

Unit test
after adopted this PR, DM can pass CI https://ci2.pingcap.net/blue/organizations/jenkins/dm_ghpr_integration_test/detail/dm_ghpr_integration_test/2843/pipeline

Code changes

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

Side effects

Related changes

Release note

 `None`.

ti-chi-bot · 2022-01-18T04:16:56Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

GMHDBJD
amyangfei

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.

lance6716 · 2022-01-18T05:19:08Z

/cc @GMHDBJD @amyangfei @liuzix

codecov-commenter · 2022-01-18T06:28:03Z

Codecov Report

Merging #4376 (0117a7a) into master (ab62e6b) will decrease coverage by 0.1013%.
The diff coverage is 45.0581%.

Flag	Coverage Δ
cdc	`59.2862% <54.6042%> (-0.3298%)`	⬇️
dm	`52.1033% <40.9688%> (+0.0377%)`	⬆️

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

@@               Coverage Diff                @@
##             master      #4376        +/-   ##
================================================
- Coverage   55.5183%   55.4169%   -0.1014%     
================================================
  Files           495        498         +3     
  Lines         61151      61649       +498     
================================================
+ Hits          33950      34164       +214     
- Misses        23789      24063       +274     
- Partials       3412       3422        +10

lance6716 · 2022-01-18T08:02:05Z

/cc @glorv

pkg/sqlmodel/multivalue.go

liuzix · 2022-01-20T08:55:00Z

pkg/sqlmodel/row_change.go

+	if tiCtx != nil {
+		ret.tiSessionCtx = tiCtx
+	} else {
+		ret.tiSessionCtx = utils.NewSessionCtx(nil)


Can you check the cost of NewSessionCtx?

changed to use a global variable

amyangfei · 2022-01-19T03:09:12Z

dm/pkg/utils/common.go

+	variables.Rng.SetSeed1(0)
+	variables.Rng.SetSeed2(0)


What are these two lines used for?

Removed now. Originally I'm use different session context instance for each RowChange, their random seed fields are different so can't pass DeepEquals

lance6716 · 2022-01-20T09:58:01Z

I think this library can provide different Causality function like RowCausality, TableCausality 🤔

type CausalityKeyFunc func (change *RowChange) []string

Will add it in future PR because I want to reduce divergence with #4313

overvenus · 2022-01-20T15:24:23Z

pkg/sqlmodel/multivalue.go

+	"github.com/pingcap/tiflow/pkg/quotes"
+)
+
+// SameTypeTargetAndColumns check whether two row changes have same type, target and columns, so they can be merged to a


Please keep line width within 80 chars.

overvenus · 2022-01-21T08:53:48Z

pkg/sqlmodel/row_change.go

+	r.identityInfo = schema.GetDownStreamTI(r.targetTableInfo, r.sourceTableInfo)
+}
+
+func getColsAndValuesOfIdx(columns []*timodel.ColumnInfo, indexColumns *timodel.IndexInfo, data []interface{}) ([]*timodel.ColumnInfo, []interface{}) {


Please keep line width within 80 chars.

1 table = 4 spaces.

lance6716 · 2022-01-25T09:10:11Z

ping @glorv

lance6716 · 2022-02-07T02:35:06Z

ping @amyangfei @liuzix @GMHDBJD

GMHDBJD

LGTM

lance6716 · 2022-02-07T08:36:53Z

/merge

ti-chi-bot · 2022-02-07T08:36:55Z

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

Commit hash: d58b935

lance6716 · 2022-02-07T08:43:15Z

/check-issue-triage-complete

lance6716 · 2022-02-07T10:27:49Z

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

lance6716 · 2022-02-07T10:49:53Z

/hold

test failed

lance6716 · 2022-02-07T12:37:59Z

after DPanic takes effect, I found a problem f2c4bcb

PTAL @GMHDBJD and /merge if looks good

lance6716 · 2022-02-07T12:41:18Z

/run-dm-integration-test

lance6716 · 2022-02-08T02:11:48Z

/unhold
/merge

ti-chi-bot · 2022-02-08T02:11:50Z

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

Commit hash: f2c4bcb

lance6716 · 2022-02-08T03:09:21Z

/run-kafka-integration-test

pkg(both): a seperate library for DML row change

a15e9ec

lance6716 added area/dm Issues or PRs related to DM. area/ticdc Issues or PRs related to TiCDC. labels Jan 18, 2022

fix UT

32afbc7

ti-chi-bot requested review from amyangfei, GMHDBJD and liuzix January 18, 2022 05:19

lance6716 changed the title ~~pkg(both): a seperate library for DML row change~~ pkg(both): a separate library for DML row change Jan 18, 2022

try to fix test race

d2c1a95

ti-chi-bot requested a review from glorv January 18, 2022 08:02

liuzix reviewed Jan 20, 2022

View reviewed changes

amyangfei reviewed Jan 20, 2022

View reviewed changes

address comment

0117a7a

overvenus reviewed Jan 20, 2022

View reviewed changes

80 char per line

b9d2a8c

overvenus reviewed Jan 21, 2022

View reviewed changes

wrap more lines to 80 chars per line

39395c1

lance6716 requested a review from amyangfei January 25, 2022 02:30

amyangfei approved these changes Feb 7, 2022

View reviewed changes

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

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

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

Merge branch 'master' into dmlmodel

d3511d5

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

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

ti-chi-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 7, 2022

fix CI

f2c4bcb

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

ti-chi-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 8, 2022

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

ti-chi-bot added 2 commits February 8, 2022 10:11

Merge branch 'master' into dmlmodel

e4b2b8f

Merge branch 'master' into dmlmodel

261678a

ti-chi-bot merged commit 3c410c5 into pingcap:master Feb 8, 2022

lance6716 mentioned this pull request Feb 8, 2022

syncer(dm): use DML library #4313

Merged

lance6716 deleted the dmlmodel branch October 13, 2022 08:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg(both): a separate library for DML row change #4376

pkg(both): a separate library for DML row change #4376

lance6716 commented Jan 18, 2022 •

edited

Loading

ti-chi-bot commented Jan 18, 2022 •

edited

Loading

lance6716 commented Jan 18, 2022

codecov-commenter commented Jan 18, 2022 •

edited

Loading

lance6716 commented Jan 18, 2022

liuzix Jan 20, 2022

lance6716 Jan 20, 2022

amyangfei Jan 19, 2022

lance6716 Jan 20, 2022 •

edited

Loading

lance6716 commented Jan 20, 2022

overvenus Jan 20, 2022

overvenus Jan 21, 2022

overvenus Jan 21, 2022 •

edited

Loading

lance6716 commented Jan 25, 2022

lance6716 commented Feb 7, 2022

GMHDBJD left a comment

lance6716 commented Feb 7, 2022

ti-chi-bot commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 8, 2022

ti-chi-bot commented Feb 8, 2022

lance6716 commented Feb 8, 2022

pkg(both): a separate library for DML row change #4376

pkg(both): a separate library for DML row change #4376

Conversation

lance6716 commented Jan 18, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jan 18, 2022 • edited Loading

lance6716 commented Jan 18, 2022

codecov-commenter commented Jan 18, 2022 • edited Loading

Codecov Report

lance6716 commented Jan 18, 2022

liuzix Jan 20, 2022

Choose a reason for hiding this comment

lance6716 Jan 20, 2022

Choose a reason for hiding this comment

amyangfei Jan 19, 2022

Choose a reason for hiding this comment

lance6716 Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

lance6716 commented Jan 20, 2022

overvenus Jan 20, 2022

Choose a reason for hiding this comment

overvenus Jan 21, 2022

Choose a reason for hiding this comment

overvenus Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

lance6716 commented Jan 25, 2022

lance6716 commented Feb 7, 2022

GMHDBJD left a comment

Choose a reason for hiding this comment

lance6716 commented Feb 7, 2022

ti-chi-bot commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 7, 2022

lance6716 commented Feb 8, 2022

ti-chi-bot commented Feb 8, 2022

lance6716 commented Feb 8, 2022

lance6716 commented Jan 18, 2022 •

edited

Loading

ti-chi-bot commented Jan 18, 2022 •

edited

Loading

codecov-commenter commented Jan 18, 2022 •

edited

Loading

lance6716 Jan 20, 2022 •

edited

Loading

overvenus Jan 21, 2022 •

edited

Loading