kafka: verify replication-factor when need to create the topic - #5715
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughKafka sink verification now conditionally creates missing topics and always constructs the event encoder afterward. Kafka topic metadata includes the minimum partition replication factor, which is used when validating ChangesKafka sink validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Verify
participant KafkaAdmin
participant EventEncoder
Verify->>KafkaAdmin: fetch target topic metadata
Verify->>KafkaAdmin: create missing topic when AutoCreate is enabled
Verify->>EventEncoder: construct event encoder
EventEncoder-->>Verify: return encoder or Schema Registry error
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/test all |
|
/test all |
|
/test all |
|
/retest |
|
/test all |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@downstreamadapter/sink/kafka/sink.go`:
- Line 146: The Kafka sink must initialize the event encoder before any Kafka
interaction so Schema Registry errors are returned first. In
downstreamadapter/sink/kafka/sink.go lines 146-146, move codec.NewEventEncoder
after claim-check setup and before kafka.NewSaramaFactory/AdminClient or topic
metadata checks. In downstreamadapter/sink/kafka/sink_test.go lines 43-77,
update the test’s broker mock/assertions to fail if Kafka is contacted before
the Schema Registry error is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f19d4d7-7a16-4bd0-9044-08cbdbd5fb59
📒 Files selected for processing (4)
downstreamadapter/sink/kafka/sink.godownstreamadapter/sink/kafka/sink_test.gopkg/sink/kafka/admin_test.gopkg/sink/kafka/options.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/sink/kafka/options.go
|
/test all |
[LGTM Timeline notifier]Timeline:
|
|
/test pull-cdc-kafka-integration-light |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asddongmen, hongyunyan, wk989898 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick release-8.5 |
|
@3AceShowHand: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: close #5720
Kafka sink
Verifyreturned immediately after fetching metadata when the target topic already existed. This skippedNewEventEncoder, so encoder-owned checks—most notably Avro Schema Registry connectivity—were not run.replication-factoris a topic creation option. The previous path could validate it even when the topic already existed or automatic topic creation was disabled, although TiCDC would not use it in either case.What is changed and how it works?
masterand remove the early return for an existing topic. Encoder initialization now runs after Kafka verification for both existing and missing topics.required-acks=all, compare the configured replication factor with the brokermin.insync.replicas;CreateTopicrequest during sink verification.replication-factorwhile applying sink options.Check List
Tests
Unit test
go test --tags=intest ./pkg/sink/kafka ./downstreamadapter/sink/kafka ./downstreamadapter/sink/topicmanager -count=1Questions
Will it cause performance regression or break compatibility?
No. This changes verification and topic-creation validation only. It does not change a public API or protocol.
Do you need to update user documentation, design documentation, or monitoring documentation?
No.
Release note