Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avro supports handle key(s) as Kafka key, and other miscellaneous improvements #862

Merged
merged 15 commits into from
Aug 26, 2020

Conversation

liuzix
Copy link
Contributor

@liuzix liuzix commented Aug 19, 2020

What problem does this PR solve?

  • Lack of deletion support via Avro
  • Bugs in MySQL TIME type with Avro

Check List

Tests

  • Unit test
  • Integration test

Release note

  • No release note

@liuzix liuzix added the component/sink Sink component. label Aug 19, 2020
@liuzix liuzix requested review from amyangfei and zier-one August 19, 2020 18:30
@liuzix liuzix force-pushed the zixiong-avro-delete branch from 8269b05 to c6af3da Compare August 20, 2020 07:04
@liuzix
Copy link
Contributor Author

liuzix commented Aug 20, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Aug 20, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Aug 20, 2020

/run-all-tests

@liuzix liuzix added the status/ptal Could you please take a look? label Aug 20, 2020
@liuzix
Copy link
Contributor Author

liuzix commented Aug 20, 2020

/run-all-tests

@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2020

Codecov Report

Merging #862 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master       #862   +/-   ##
===========================================
  Coverage   34.9028%   34.9028%           
===========================================
  Files            97         97           
  Lines         11681      11681           
===========================================
  Hits           4077       4077           
  Misses         7195       7195           
  Partials        409        409           

cdc/sink/codec/avro.go Outdated Show resolved Hide resolved
cdc/sink/codec/avro.go Outdated Show resolved Hide resolved
Comment on lines 336 to 338
if v, ok := col.Value.(uint64); ok {
col.Value = int64(v)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conversion will lose precision, does Avro have no support for uint64?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of Avro only supports sign integers, and passing unsigned types will cause panics. Avro should support unsigned integers in theory.

@liuzix
Copy link
Contributor Author

liuzix commented Aug 25, 2020

/run-all-tests

1 similar comment
@liuzix
Copy link
Contributor Author

liuzix commented Aug 25, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Aug 25, 2020

/run-all-tests

1 similar comment
@liuzix
Copy link
Contributor Author

liuzix commented Aug 25, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Aug 25, 2020

/run-all-tests

@liuzix
Copy link
Contributor Author

liuzix commented Aug 25, 2020

/run-all-tests

@amyangfei
Copy link
Contributor

/run-integration-tests

Copy link
Contributor

@amyangfei amyangfei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -55,113 +55,130 @@ const (
MultipleKeyFlag
// NullableFlag means the column is nullable
NullableFlag
// UnsignedFlag means the column stores an unsigned integer
UnsignedFlag
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to add this flag to document

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 26, 2020
@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Aug 26, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Aug 26, 2020
@amyangfei
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 26, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 5a26d14 into pingcap:master Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/sink Sink component. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. status/ptal Could you please take a look?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants