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

Kafka Connect Avro Support Part 2: AvroEventBatchEncoder #733

Merged
merged 9 commits into from
Jul 14, 2020

Conversation

liuzix
Copy link
Contributor

@liuzix liuzix commented Jul 8, 2020

What problem does this PR solve?

#660

What is changed and how it works?

Add AvroEventBatchEncoder which handles RowChangedEvent and DDLEvent.

Check List

Tests

  • Unit test
  • Integration test: will be done in the next stage after the sink is ready.

Code changes

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

TODO:

  • Use primary keys as Kafka key.

Release note

  • No release note

@liuzix liuzix changed the title Kafka Connect Avro Support Part 1: AvroEventBatchEncoder Kafka Connect Avro Support Part 2: AvroEventBatchEncoder Jul 8, 2020
pm: m,
startTs: startTs,
}
//return nil
Copy link
Member

Choose a reason for hiding this comment

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

Please remove it.


package codec

import (
Copy link
Member

Choose a reason for hiding this comment

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

import order should be

std libs
<new line>
third-party libs

"time"
)

type AvroBatchEncoderSuite struct {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type AvroBatchEncoderSuite struct {
type avroBatchEncoderSuite struct {

Please avoid unnecessary exports.

@@ -44,7 +44,7 @@ type mockRegistrySchema struct {
ID int
}

func (s *AvroSchemaRegistrySuite) SetUpSuite(c *check.C) {
func StartHTTPInterceptForTestingRegistry(c *check.C) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func StartHTTPInterceptForTestingRegistry(c *check.C) {
func startHTTPInterceptForTestingRegistry(c *check.C) {

@@ -140,10 +140,18 @@ func (s *AvroSchemaRegistrySuite) SetUpSuite(c *check.C) {

}

func (s *AvroSchemaRegistrySuite) TearDownSuite(c *check.C) {
func StopHTTPInterceptForTestingRegistry() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func StopHTTPInterceptForTestingRegistry() {
func stopHTTPInterceptForTestingRegistry() {

return "long.timestamp-millis"
default:
log.Warn("getAvroDataTypeName: unknown type")
return "errorType"
Copy link
Member

Choose a reason for hiding this comment

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

Is "errorType" valid in Avro?

cdc/changefeed.go Outdated Show resolved Hide resolved
cdc/sink/codec/avro.go Outdated Show resolved Hide resolved
cdc/sink/codec/avro.go Outdated Show resolved Hide resolved
cdc/sink/codec/avro.go Outdated Show resolved Hide resolved
@amyangfei amyangfei added component/sink Sink component. status/ptal Could you please take a look? labels Jul 10, 2020
@amyangfei
Copy link
Contributor

/lgtm

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 13, 2020
@ti-srebot
Copy link
Contributor

@amyangfei,Thanks for your review.

Schema string
SchemaID int64
Table string
ColumnInfo []*ColumnInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could use a TableInfo with a table name, schema name, column info, etc.

Copy link
Contributor

@zier-one zier-one left a comment

Choose a reason for hiding this comment

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

@zier-one
Copy link
Contributor

/lgtm

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jul 14, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jul 14, 2020
@zier-one
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 14, 2020
@zier-one zier-one removed the status/ptal Could you please take a look? label Jul 14, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@liuzix merge failed.

@amyangfei
Copy link
Contributor

Please fix the build error @liuzix

@zier-one
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@zier-one
Copy link
Contributor

/merge

1 similar comment
@zier-one
Copy link
Contributor

/merge

@ti-srebot
Copy link
Contributor

@liuzix merge failed.

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 18b8319 into pingcap:master Jul 14, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants