-
Notifications
You must be signed in to change notification settings - Fork 60
sink: add debezium-avro protocol #5475
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
Changes from all commits
d14037f
3f4d302
4e9e16c
0c4c082
d47b0e8
dec3aad
79d7780
73f9bce
27329ea
1b5db07
f2aa423
3c875f6
4333614
f94ff37
d0a7cd2
3d9fbc7
c0e1a61
b961dd9
f132699
4d48de3
6879c93
29d29a1
027cf77
15955a4
ed1b9f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -42,28 +42,48 @@ func getPartitionNum(o *option) (int32, error) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| defer admin.Close() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| topics := strings.Split(o.topic, ",") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| maxPartitionNum := int32(0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout := 3000 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i := 0; i <= 30; i++ { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resp, err := admin.GetMetadata(&o.topic, false, timeout) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err.(kafka.Error).Code() == kafka.ErrTransport { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Info("retry get partition number", zap.Int("retryTime", i), zap.Int("timeout", timeout)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout += 100 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for _, topic := range topics { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| topic = strings.TrimSpace(topic) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if topic == "" { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| found := false | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for i := 0; i <= 30; i++ { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resp, err := admin.GetMetadata(&topic, false, timeout) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var kafkaErr kafka.Error | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if errors.As(err, &kafkaErr) && kafkaErr.Code() == kafka.ErrTransport { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Info("retry get partition number", zap.String("topic", topic), zap.Int("retryTime", i), zap.Int("timeout", timeout)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeout += 100 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0, errors.Trace(err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if topicDetail, ok := resp.Topics[topic]; ok { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| numPartitions := int32(len(topicDetail.Partitions)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Info("get partition number of topic", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| zap.String("topic", topic), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| zap.Int32("partitionNum", numPartitions)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if numPartitions > maxPartitionNum { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| maxPartitionNum = numPartitions | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| found = true | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| break | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+65
to
75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify existing metadata error handling and the Kafka client package in use.
rg -n 'confluent-kafka-go|IBM/sarama' go.mod go.sum
rg -n 'GetMetadata\(|topicDetail\.Error|TopicMetadata' --type go .Repository: pingcap/ticdc Length of output: 1002 🏁 Script executed: #!/bin/bash
# Inspect the admin.go file to understand the GetMetadata implementation and return type.
cat -n pkg/sink/kafka/admin.go | head -80Repository: pingcap/ticdc Length of output: 3079 🏁 Script executed: #!/bin/bash
# Check the sarama TopicMetadata struct definition in vendor or go mod cache (simulated by checking if vendor exists or just looking at the struct usage pattern in the codebase).
# Since we can't access go mod cache directly in a simple script, we will search for the struct usage in the codebase.
rg -n 'type TopicMetadata struct' --type go .Repository: pingcap/ticdc Length of output: 151 🏁 Script executed: #!/bin/bash
# Check the usage of GetMetadata response in consumer.go around line 55 to see the type of 'resp'.
cat -n cmd/kafka-consumer/consumer.go | sed -n '50,80p'Repository: pingcap/ticdc Length of output: 1371 🏁 Script executed: #!/bin/bash
# Check the import in consumer.go and search for TopicMetadata struct details in the codebase or vendor logic.
rg -A 5 'type.*TopicMetadata' --type go .
rg -A 10 'type.*MetadataResponse' --type go .Repository: pingcap/ticdc Length of output: 151 🏁 Script executed: #!/bin/bash
# Check if there is a local vendor or if we can find the sarama package definition via grep in the mod cache simulation or just assume standard sarama if not found locally.
# Let's try to find the definition of the error field in the TopicMetadata struct by searching for its usage with .Err or .Error.
rg -n 'topicDetail\.Err|TopicMetadata.*Err' --type go .Repository: pingcap/ticdc Length of output: 151 🏁 Script executed: #!/bin/bash
# Check imports and admin variable declaration in consumer.go
cat -n cmd/kafka-consumer/consumer.go | head -40Repository: pingcap/ticdc Length of output: 1632 🏁 Script executed: #!/bin/bash
# Find where GetMetadata is defined or used to determine the return type.
rg -n 'func.*GetMetadata' --type go .
rg -n 'MetadataResponse' --type go .Repository: pingcap/ticdc Length of output: 151 🏁 Script executed: #!/bin/bash
# Since local src code for sarama/confuent structs might not be present, search for any existing usage of .Err on metadata in the codebase to confirm the pattern.
rg -n '\.Err' --type go cmd/kafka-consumer/Repository: pingcap/ticdc Length of output: 3089 🏁 Script executed: #!/bin/bash
# Check the go.mod to confirm the exact version and search for any existing error handling patterns in the consumer file.
grep -A 5 'confluent-kafka-go' go.mod
rg -n 'kafka.Error' cmd/kafka-consumer/consumer.goRepository: pingcap/ticdc Length of output: 442 Check per-topic metadata errors before treating the topic as found.
The code uses 💡 Suggested fixCheck if topicDetail, ok := resp.Topics[topic]; ok {
+ if topicDetail.Err != kafka.ErrNoError {
+ log.Info("retry get partition number due to topic error",
+ zap.String("topic", topic),
+ zap.Error(topicDetail.Err))
+ time.Sleep(1 * time.Second)
+ continue
+ }
numPartitions := int32(len(topicDetail.Partitions))📝 Committable suggestion
Suggested change
🧰 Tools🪛 ast-grep (0.44.0)[warning] 65-65: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values. (integer-overflow-narrowing-conversion-go) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0, errors.Trace(err) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Info("retry get partition number", zap.String("topic", topic)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| time.Sleep(1 * time.Second) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if topicDetail, ok := resp.Topics[o.topic]; ok { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| numPartitions := int32(len(topicDetail.Partitions)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Info("get partition number of topic", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| zap.String("topic", o.topic), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| zap.Int32("partitionNum", numPartitions)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return numPartitions, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !found { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0, errors.Errorf("get partition number(%s) timeout", topic) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| log.Info("retry get partition number", zap.String("topic", o.topic)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| time.Sleep(1 * time.Second) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0, errors.Errorf("get partition number(%s) timeout", o.topic) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if maxPartitionNum == 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return 0, errors.Errorf("get partition number(%s) timeout", o.topic) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return maxPartitionNum, nil | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type consumer struct { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -320,44 +320,51 @@ func TestOnDDLMarksRoutedCreateTableLikePartitionTableForAvro(t *testing.T) { | |||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| func TestAppendRow2GroupKeepsDebeziumPartitionTableFallback(t *testing.T) { | ||||||||||||||||
| replicaCfg := config.GetDefaultReplicaConfig() | ||||||||||||||||
| eventRouter, err := eventrouter.NewEventRouter(replicaCfg.Sink, "test-topic", false, false) | ||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||
| for _, protocol := range []config.Protocol{ | ||||||||||||||||
| config.ProtocolDebezium, | ||||||||||||||||
| config.ProtocolDebeziumAvro, | ||||||||||||||||
| } { | ||||||||||||||||
| t.Run(protocol.String(), func(t *testing.T) { | ||||||||||||||||
| replicaCfg := config.GetDefaultReplicaConfig() | ||||||||||||||||
| eventRouter, err := eventrouter.NewEventRouter(replicaCfg.Sink, "test-topic", false, false) | ||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||
|
Comment on lines
+328
to
+330
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Use the Avro-like router flag in the Debezium-Avro subtest.
Suggested fix- eventRouter, err := eventrouter.NewEventRouter(replicaCfg.Sink, "test-topic", false, false)
+ isAvroLike := protocol == config.ProtocolDebeziumAvro
+ eventRouter, err := eventrouter.NewEventRouter(replicaCfg.Sink, "test-topic", false, isAvroLike)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
|
|
||||||||||||||||
| w := &writer{ | ||||||||||||||||
| progresses: []*partitionProgress{{partition: 0, eventsGroup: make(map[int64]*util.EventsGroup)}}, | ||||||||||||||||
| eventRouter: eventRouter, | ||||||||||||||||
| protocol: config.ProtocolDebezium, | ||||||||||||||||
| partitionTableAccessor: codeccommon.NewPartitionTableAccessor(), | ||||||||||||||||
| } | ||||||||||||||||
| w := &writer{ | ||||||||||||||||
| progresses: []*partitionProgress{{partition: 0, eventsGroup: make(map[int64]*util.EventsGroup)}}, | ||||||||||||||||
| eventRouter: eventRouter, | ||||||||||||||||
| protocol: protocol, | ||||||||||||||||
| partitionTableAccessor: codeccommon.NewPartitionTableAccessor(), | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| w.partitionTableAccessor.Add("target", "src") | ||||||||||||||||
| ddl := &commonEvent.DDLEvent{ | ||||||||||||||||
| Query: "CREATE TABLE `target`.`dst` LIKE `target`.`src`", | ||||||||||||||||
| SchemaName: "target", | ||||||||||||||||
| TableName: "dst", | ||||||||||||||||
| Type: byte(timodel.ActionCreateTable), | ||||||||||||||||
| } | ||||||||||||||||
| w.onDDL(ddl) | ||||||||||||||||
| require.True(t, w.partitionTableAccessor.IsPartitionTable("target", "dst")) | ||||||||||||||||
| w.partitionTableAccessor.Add("target", "src") | ||||||||||||||||
| ddl := &commonEvent.DDLEvent{ | ||||||||||||||||
| Query: "CREATE TABLE `target`.`dst` LIKE `target`.`src`", | ||||||||||||||||
| SchemaName: "target", | ||||||||||||||||
| TableName: "dst", | ||||||||||||||||
| Type: byte(timodel.ActionCreateTable), | ||||||||||||||||
| } | ||||||||||||||||
| w.onDDL(ddl) | ||||||||||||||||
| require.True(t, w.partitionTableAccessor.IsPartitionTable("target", "dst")) | ||||||||||||||||
|
|
||||||||||||||||
| newDMLEvent := func(commitTs uint64) *commonEvent.DMLEvent { | ||||||||||||||||
| return &commonEvent.DMLEvent{ | ||||||||||||||||
| PhysicalTableID: 1, | ||||||||||||||||
| CommitTs: commitTs, | ||||||||||||||||
| RowTypes: []common.RowType{common.RowTypeUpdate}, | ||||||||||||||||
| Rows: chunk.NewChunkWithCapacity(nil, 0), | ||||||||||||||||
| TableInfo: &common.TableInfo{ | ||||||||||||||||
| TableName: common.TableName{Schema: "target", Table: "dst"}, | ||||||||||||||||
| }, | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| newDMLEvent := func(commitTs uint64) *commonEvent.DMLEvent { | ||||||||||||||||
| return &commonEvent.DMLEvent{ | ||||||||||||||||
| PhysicalTableID: 1, | ||||||||||||||||
| CommitTs: commitTs, | ||||||||||||||||
| RowTypes: []common.RowType{common.RowTypeUpdate}, | ||||||||||||||||
| Rows: chunk.NewChunkWithCapacity(nil, 0), | ||||||||||||||||
| TableInfo: &common.TableInfo{ | ||||||||||||||||
| TableName: common.TableName{Schema: "target", Table: "dst"}, | ||||||||||||||||
| }, | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| progress := w.progresses[0] | ||||||||||||||||
| w.appendRow2Group(newDMLEvent(200), progress, kafka.Offset(10)) | ||||||||||||||||
| w.appendRow2Group(newDMLEvent(100), progress, kafka.Offset(11)) | ||||||||||||||||
| progress := w.progresses[0] | ||||||||||||||||
| w.appendRow2Group(newDMLEvent(200), progress, kafka.Offset(10)) | ||||||||||||||||
| w.appendRow2Group(newDMLEvent(100), progress, kafka.Offset(11)) | ||||||||||||||||
|
|
||||||||||||||||
| resolved := progress.eventsGroup[1].ResolveInto(150, nil) | ||||||||||||||||
| require.Len(t, resolved, 1) | ||||||||||||||||
| require.Equal(t, uint64(100), resolved[0].CommitTs) | ||||||||||||||||
| resolved := progress.eventsGroup[1].ResolveInto(150, nil) | ||||||||||||||||
| require.Len(t, resolved, 1) | ||||||||||||||||
| require.Equal(t, uint64(100), resolved[0].CommitTs) | ||||||||||||||||
| }) | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -491,8 +491,9 @@ func (info *ChangeFeedInfo) RmUnusedFields() { | |||||||||||||||||||||
| info.rmMQOnlyFields() | ||||||||||||||||||||||
| } else { | ||||||||||||||||||||||
| // remove schema registry for MQ downstream with | ||||||||||||||||||||||
| // protocol other than avro | ||||||||||||||||||||||
| if util.GetOrZero(info.Config.Sink.Protocol) != ProtocolAvro.String() { | ||||||||||||||||||||||
| // protocol other than avro or debezium-avro | ||||||||||||||||||||||
| protocol := util.GetOrZero(info.Config.Sink.Protocol) | ||||||||||||||||||||||
| if protocol != ProtocolAvro.String() && protocol != ProtocolDebeziumAvro.String() { | ||||||||||||||||||||||
|
Comment on lines
493
to
+496
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Normalize the protocol before deciding whether to drop Line 496 compares the raw sink string, so Suggested fix- protocol := util.GetOrZero(info.Config.Sink.Protocol)
- if protocol != ProtocolAvro.String() && protocol != ProtocolDebeziumAvro.String() {
+ protocol, err := ParseSinkProtocolFromString(util.GetOrZero(info.Config.Sink.Protocol))
+ if err != nil || (protocol != ProtocolAvro && protocol != ProtocolDebeziumAvro) {
info.Config.Sink.SchemaRegistry = nil
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| info.Config.Sink.SchemaRegistry = nil | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
Repository: pingcap/ticdc
Length of output: 1886
Avoid disabling
G115repo-wide._test.gofiles and thetests/directory are already fully excluded fromgosec(Lines 114–129), so this change specifically removes integer overflow checks (G115) from production code. This weakens security posture without justification, as the "noise" is likely confined to test files which are already ignored.Scope this exclusion to specific paths or use
//nolint:gosecannotations at the specific call sites instead of disabling the rule globally.🤖 Prompt for AI Agents