kafka: make the verify lightweight - #5617
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughKafka sink verification now performs explicit protocol, encoder, admin, metadata, and topic-creation validation. Kafka component construction uses a direct Sarama factory, while consumer partition discovery checks metadata error codes. ChangesKafka sink verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Verify
participant EventRouter
participant ColumnSelector
participant SaramaFactory
participant KafkaAdmin
participant EventEncoder
Verify->>EventRouter: initialize Avro-like routing
Verify->>ColumnSelector: initialize column selection
Verify->>SaramaFactory: create admin client
SaramaFactory->>KafkaAdmin: fetch topic metadata
KafkaAdmin-->>Verify: return topic metadata
Verify->>KafkaAdmin: create missing topic when enabled
Verify->>EventEncoder: create and clean encoder
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the Verify function in the Kafka sink to perform comprehensive validation of configurations (such as protocol, topic, options, encoder, event router, and column selector) without initializing a full sink component, and adds a corresponding unit test. The review feedback suggests two important improvements to the validation logic: first, ensuring that the user-specified partition count does not exceed the actual partition count of an existing topic; and second, defaulting the partition count to 3 during dry-run topic creation validation if it is unspecified (<= 0) to prevent Kafka broker validation failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/test all |
[LGTM Timeline notifier]Timeline:
|
|
[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 |
|
/retest |
1 similar comment
|
/retest |
|
/test all |
|
/test all |
|
/test all |
|
/test all |
|
/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 #5618
Kafka sink
Verifyreused the normal sink construction path. That path creates runtime components such as the encoder group and topic manager, and can perform startup-only topic creation work. Verification should stay lightweight: validate user configuration, construct only cheap local helpers, and use Kafka admin APIs only for read-only or validate-only checks.What is changed and how it works?
Verifyto run the shared parse/config checks directly:GetProtocolGetTopicoptions.ApplyGetEncoderConfig(...).Validate()eventrouter.NewEventRoutercolumnselector.Newcodec.NewEventEncodercall to reject protocols that Kafka sink does not supportGetTopicsMetaauto-create-topic=false: return a config errorauto-create-topic=true: callCreateTopic(..., validateOnly=true)to validate parameters and permissions without creating the topicNew/ topic manager startup.Check List
Tests
Commands run locally:
Questions
Will it cause performance regression or break compatibility?
No performance regression is expected. This makes
Verifylighter and removes startup-only side effects from verification. Sink startup behavior and real topic creation remain unchanged.Do you need to update user documentation, design documentation or monitoring documentation?
No. This changes internal verification behavior only; no user-facing config, protocol, metric, or documentation surface changes.
Release note
Summary by CodeRabbit