codec(ticdc): fix some avro encoding bugs #4704

zhangyangyu · 2022-02-25T08:18:57Z

What problem does this PR solve?

Issue Number: close #4454, close #4453, close #4420

What is changed and how it works?

To resolve the issues, we need to change the codec data mapping for enum/set(->string) and string/binary. This requires knowing table definition information when performing codec. So I carry the information in RowChangedEvent.

Release note

Fix the wrong data mapping for avro codec of type Enum/Set and TinyText/MediumText/Text/LongText.

ti-chi-bot · 2022-02-25T08:18:59Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

liuzix
overvenus

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.

overvenus

Could you add tests to cover changes in avro.go? Thanks!

cdc/entry/mounter.go

cdc/sink/codec/avro.go

overvenus · 2022-02-25T09:19:32Z

cdc/sink/codec/avro.go

@@ -312,6 +312,9 @@ func getAvroDataTypeFromColumn(col *model.Column) (interface{}, error) {
 	case mysql.TypeDouble:
 		return "double", nil
 	case mysql.TypeVarchar, mysql.TypeString, mysql.TypeVarString:
+		if col.Flag.IsBinary() {


Could you add some unit tests?

In the future, maybe we need a flag to control the behavior? Like https://debezium.io/documentation/reference/connectors/mysql.html#mysql-property-binary-handling-mode

Cc @leoppro

zhangyangyu · 2022-02-25T10:00:01Z

/hold

working on more tests

codecov-commenter · 2022-02-27T08:54:38Z

Codecov Report

Merging #4704 (8acc74f) into master (9607554) will decrease coverage by 0.3831%.
The diff coverage is 52.6048%.

Flag	Coverage Δ
cdc	`59.9368% <52.6048%> (+0.0145%)`	⬆️
dm	`51.3419% <ø> (-0.6870%)`	⬇️

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

@@               Coverage Diff                @@
##             master      #4704        +/-   ##
================================================
- Coverage   55.6402%   55.2570%   -0.3832%     
================================================
  Files           494        518        +24     
  Lines         61283      63914      +2631     
================================================
+ Hits          34098      35317      +1219     
- Misses        23750      25113      +1363     
- Partials       3435       3484        +49

zhangyangyu · 2022-02-27T15:14:38Z

/unhold

zhangyangyu · 2022-02-27T15:35:02Z

/run-all-tests

lonng · 2022-02-28T05:43:32Z

cdc/model/sink.go

@@ -253,7 +254,8 @@ type RowChangedEvent struct {

 	RowID int64 `json:"row-id" msg:"-"` // Deprecated. It is empty when the RowID comes from clustered index table.

-	Table *TableName `json:"table" msg:"table"`
+	Table    *TableName         `json:"table" msg:"table"`
+	ColInfos []rowcodec.ColInfo `json:"column-infos" msg:"-"`


Please use column_infos.

Sorry I don't really get it. Do you mean change the json:"column-infos" to json:"column_infos"? But all other fields use hyphens instead of underscores.

msg:"-" seems to prevent the information from being persisted for Redo. Could you verify the compatibility of this change with Redo?

Redo only applies to mysql sink currently so does none of avro business. So prevent ColInfos from being persisted for Redo seems right.

But I think the ColInfos is important and maybe in future Redo could support avro? Persist ColInfos seems bring no trouble. Upgrade is also safe since a nil ColInfos field is safe for other codecs. But as @liuzix tells me, it will make the redo log large. So I'd like to leave it unpersist in this pull request.

Could you take a look @ben1009 ?

lgtm for redo part

overvenus

Rest LGTM

cdc/sink/codec/avro.go

Rustin170506

Do we need to cp it to 5.4, it looks like you only marked 5.3?

Please write a more detailed description of your PRs, how they work and why they were changed. Thanks!

Also, please add the release note.

zhangyangyu · 2022-02-28T11:42:19Z

/run-all-tests

zhangyangyu · 2022-02-28T11:59:59Z

Do we need to cp it to 5.4, it looks like you only marked 5.3?
Please write a more detailed description of your PRs, how they work and why they were changed. Thanks!
Also, please add the release note.

All the three issues are marked severity/moderate. Usually we don't cherry-pick such PRs. Actually the behaviour affects all versions from v4.0. I will consult the PM.

zhangyangyu · 2022-02-28T12:31:17Z

/run-all-tests

zhangyangyu · 2022-02-28T13:53:58Z

/run-kafka-integration-test

Rustin170506

Thanks!

Rustin170506 · 2022-03-01T08:47:41Z

cdc/sink/codec/canal_test.go

 	for _, item := range testColumnsTable {
 		obtainedMySQLType := getMySQLType(item.column)
 		c.Assert(obtainedMySQLType, check.Equals, item.expectedMySQLType)

 		obtainedJavaSQLType, err := getJavaSQLType(item.column, obtainedMySQLType)
 		c.Assert(err, check.IsNil)
 		c.Assert(obtainedJavaSQLType, check.Equals, item.expectedJavaSQLType)
+
+		if !item.column.Flag.IsBinary() {


Why only for binary?

For non-binary. Because the expectedValue is dummy for binary: https://github.com/pingcap/tiflow/blob/master/cdc/sink/codec/canal_test.go#L424.

Rustin170506 · 2022-03-01T08:50:40Z

cdc/sink/codec/avro_test.go

-	}, time.Local)
+	}
+
+	colInfos := []rowcodec.ColInfo{


Maybe we can organize this data by structured arrays? Controlling correspondence by index may be difficult with large test data.

Yes. Actually using arrays restricts the usage pattern. We have to iterate the array(currently no problem). I tried to use the tp map, but it can't be used since the column Id is lost in Column struct. Add fields to Column will cause much code churn. Any suggestions?

zhangyangyu · 2022-03-01T12:57:55Z

/run-all-tests

zhangyangyu · 2022-03-01T13:51:21Z

/run-verify

zhangyangyu · 2022-03-02T10:58:32Z

It's confirmed that we could just follow our conventional workflow and don't need to cherry-pick.

zhangyangyu · 2022-03-02T11:01:20Z

/remove-label needs-cherry-pick-release-5.3

overvenus · 2022-03-07T13:04:00Z

/merge

ti-chi-bot · 2022-03-07T13:04:03Z

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

Commit hash: b9b05cb

ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. needs-cherry-pick-release-5.3 Should cherry pick this PR to release-5.3 branch. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 25, 2022

zhangyangyu requested review from overvenus and disksing February 25, 2022 08:28

overvenus reviewed Feb 25, 2022

View reviewed changes

overvenus requested a review from liuzix February 25, 2022 09:44

zhangyangyu force-pushed the fix-avro-encoding branch from 4b7ec56 to f2ea25e Compare February 25, 2022 09:49

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 25, 2022

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

zhangyangyu force-pushed the fix-avro-encoding branch 2 times, most recently from 012c834 to a591ccb Compare February 27, 2022 14:51

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 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 27, 2022

lonng reviewed Feb 28, 2022

View reviewed changes

overvenus reviewed Feb 28, 2022

View reviewed changes

cdc/sink/codec/avro.go Outdated Show resolved Hide resolved

overvenus added component/sink Sink component. type/bugfix This PR fixes a bug. labels Feb 28, 2022

Rustin170506 requested review from Rustin170506 and removed request for disksing February 28, 2022 10:00

Rustin170506 reviewed Feb 28, 2022

View reviewed changes

zhangyangyu added 2 commits February 28, 2022 19:41

fix some avro encoding issues

078c562

add canal test

e85f8c3

zhangyangyu added 5 commits February 28, 2022 19:41

fix mounter test

11e8526

fix avro test

961134e

instead of carry enum values, carry colInfos and query when needed

8a296f3

fix missing }

4741fc6

add comment

14b7490

zhangyangyu force-pushed the fix-avro-encoding branch from 8acc74f to 14b7490 Compare February 28, 2022 11:42

Rustin170506 reviewed Mar 1, 2022

View reviewed changes

Merge branch 'master' into fix-avro-encoding

b9b05cb

overvenus requested a review from maxshuang March 2, 2022 09:47

ti-chi-bot removed the needs-cherry-pick-release-5.3 Should cherry pick this PR to release-5.3 branch. label Mar 2, 2022

liuzix approved these changes Mar 7, 2022

View reviewed changes

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

overvenus approved these changes Mar 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 Mar 7, 2022

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

ti-chi-bot added 2 commits March 7, 2022 21:04

Merge branch 'master' into fix-avro-encoding

ee85c59

Merge branch 'master' into fix-avro-encoding

2612e5b

ti-chi-bot merged commit 54328e4 into pingcap:master Mar 7, 2022

zhangyangyu deleted the fix-avro-encoding branch March 8, 2022 02:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

codec(ticdc): fix some avro encoding bugs #4704

codec(ticdc): fix some avro encoding bugs #4704

zhangyangyu commented Feb 25, 2022 •

edited

Loading

ti-chi-bot commented Feb 25, 2022 •

edited

Loading

overvenus left a comment

overvenus Feb 25, 2022

overvenus Feb 25, 2022

zhangyangyu commented Feb 25, 2022

codecov-commenter commented Feb 27, 2022 •

edited

Loading

zhangyangyu commented Feb 27, 2022

zhangyangyu commented Feb 27, 2022

lonng Feb 28, 2022

zhangyangyu Feb 28, 2022

liuzix Feb 28, 2022

zhangyangyu Feb 28, 2022

ben1009 Feb 28, 2022

overvenus left a comment

Rustin170506 left a comment

zhangyangyu commented Feb 28, 2022

zhangyangyu commented Feb 28, 2022 •

edited

Loading

zhangyangyu commented Feb 28, 2022

zhangyangyu commented Feb 28, 2022

Rustin170506 left a comment

Rustin170506 Mar 1, 2022

zhangyangyu Mar 1, 2022

Rustin170506 Mar 1, 2022

zhangyangyu Mar 1, 2022

zhangyangyu commented Mar 1, 2022

zhangyangyu commented Mar 1, 2022

zhangyangyu commented Mar 2, 2022

zhangyangyu commented Mar 2, 2022

overvenus commented Mar 7, 2022

ti-chi-bot commented Mar 7, 2022

codec(ticdc): fix some avro encoding bugs #4704

codec(ticdc): fix some avro encoding bugs #4704

Conversation

zhangyangyu commented Feb 25, 2022 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Release note

ti-chi-bot commented Feb 25, 2022 • edited Loading

overvenus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangyangyu commented Feb 25, 2022

codecov-commenter commented Feb 27, 2022 • edited Loading

Codecov Report

zhangyangyu commented Feb 27, 2022

zhangyangyu commented Feb 27, 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

overvenus left a comment

Choose a reason for hiding this comment

Rustin170506 left a comment

Choose a reason for hiding this comment

zhangyangyu commented Feb 28, 2022

zhangyangyu commented Feb 28, 2022 • edited Loading

zhangyangyu commented Feb 28, 2022

zhangyangyu commented Feb 28, 2022

Rustin170506 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

zhangyangyu commented Mar 1, 2022

zhangyangyu commented Mar 1, 2022

zhangyangyu commented Mar 2, 2022

zhangyangyu commented Mar 2, 2022

overvenus commented Mar 7, 2022

ti-chi-bot commented Mar 7, 2022

zhangyangyu commented Feb 25, 2022 •

edited

Loading

ti-chi-bot commented Feb 25, 2022 •

edited

Loading

codecov-commenter commented Feb 27, 2022 •

edited

Loading

zhangyangyu commented Feb 28, 2022 •

edited

Loading