Skip to content

kafka: make the verify lightweight - #5617

Merged
ti-chi-bot[bot] merged 7 commits into
pingcap:masterfrom
3AceShowHand:simplify-kafka-sink
Jul 20, 2026
Merged

kafka: make the verify lightweight#5617
ti-chi-bot[bot] merged 7 commits into
pingcap:masterfrom
3AceShowHand:simplify-kafka-sink

Conversation

@3AceShowHand

@3AceShowHand 3AceShowHand commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: close #5618

Kafka sink Verify reused 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?

  • Reworks Kafka sink Verify to run the shared parse/config checks directly:
    • GetProtocol
    • GetTopic
    • options.Apply
    • GetEncoderConfig(...).Validate()
  • Keeps only the lightweight local construction checks needed by Verify:
    • eventrouter.NewEventRouter
    • columnselector.New
    • one codec.NewEventEncoder call to reject protocols that Kafka sink does not support
  • Changes Kafka topic verification to use admin metadata plus validate-only creation:
    • existing topic: pass after GetTopicsMeta
    • missing topic with auto-create-topic=false: return a config error
    • missing topic with auto-create-topic=true: call CreateTopic(..., validateOnly=true) to validate parameters and permissions without creating the topic
  • Leaves the real sink startup path unchanged, so actual topic creation still happens in New / topic manager startup.
  • Removes the now-unneeded test-only Kafka factory creator indirection.

Check List

Tests

  • Unit test

Commands run locally:

go test ./downstreamadapter/sink/kafka -run TestVerifyRejectsKafkaUnsupportedProtocolBeforeConnecting -count=1
go test --tags=intest ./downstreamadapter/sink/kafka -count=1
git diff --check

Questions

Will it cause performance regression or break compatibility?

No performance regression is expected. This makes Verify lighter 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

None

Summary by CodeRabbit

  • Bug Fixes
    • Improved Kafka topic validation to distinguish existing topics from topics with metadata errors.
    • Kafka sink verification now validates topic configuration, creation settings, encoding, and schema-related setup before completion.
    • Improved handling of Avro and Debezium-Avro Kafka protocols during sink validation.
  • Refactor
    • Simplified Kafka sink initialization while preserving existing behavior.
    • Streamlined internal Kafka factory and error-handling logic.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 10, 2026
@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5a6b78de-d874-4314-ac1b-2858f9c984e2

📥 Commits

Reviewing files that changed from the base of the PR and between eedb5df and 3510596.

📒 Files selected for processing (2)
  • downstreamadapter/sink/kafka/sink.go
  • pkg/sink/codec/builder.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • downstreamadapter/sink/kafka/sink.go

📝 Walkthrough

Walkthrough

Kafka 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.

Changes

Kafka sink verification

Layer / File(s) Summary
Explicit verification and protocol handling
downstreamadapter/sink/kafka/sink.go, pkg/sink/codec/builder.go
Verify resolves Kafka settings, validates topic existence or creation, initializes Avro-related components, creates and cleans the encoder, and uses the unaliased error tracer.
Direct Kafka factory construction
downstreamadapter/sink/kafka/helper.go, pkg/sink/kafka/factory.go
Kafka component construction directly creates the Sarama factory, and the exported FactoryCreator type is removed.
Consumer metadata error validation
cmd/kafka-consumer/consumer.go
Partition discovery uses topic metadata only when its error code is kafka.ErrNoError.

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
Loading

Possibly related PRs

Poem

A rabbit hops through Kafka’s gate,
Checking topics, small and great.
Avro paths are neatly spun,
Missing queues are checked—or done.
Sarama hums, the encoder gleams!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The consumer metadata check and codec error-tracing change are not required by the verify simplification. Remove or justify the consumer.go and codec/builder.go edits, or split them into a separate PR if they are independent of the verify simplification.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: simplifying and lightening Kafka sink verification.
Description check ✅ Passed The description covers the problem, implementation, tests, questions, and release note, and includes the required issue reference.
Linked Issues check ✅ Passed The PR aligns with #5618 by simplifying Kafka verify and keeping validation lightweight.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread downstreamadapter/sink/kafka/sink.go
Comment thread downstreamadapter/sink/kafka/sink.go
@ti-chi-bot ti-chi-bot Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 10, 2026
@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jul 10, 2026
@3AceShowHand

Copy link
Copy Markdown
Collaborator Author

/test all

@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jul 10, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-07-10 09:08:40.292192128 +0000 UTC m=+359106.328287174: ☑️ agreed by wk989898.
  • 2026-07-10 09:20:59.753470762 +0000 UTC m=+359845.789565839: ☑️ agreed by hongyunyan.

@ti-chi-bot

ti-chi-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

[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

Details Needs approval from an approver in each of these files:
  • OWNERS [asddongmen,hongyunyan,wk989898]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@3AceShowHand

Copy link
Copy Markdown
Collaborator Author

/retest

1 similar comment
@3AceShowHand

Copy link
Copy Markdown
Collaborator Author

/retest

@3AceShowHand

Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand

Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand

Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand

Copy link
Copy Markdown
Collaborator Author

/test all

@ti-chi-bot
ti-chi-bot Bot merged commit 2667ed8 into pingcap:master Jul 20, 2026
25 checks passed
@3AceShowHand

Copy link
Copy Markdown
Collaborator Author

/cherry-pick release-8.5

@ti-chi-bot

Copy link
Copy Markdown
Member

@3AceShowHand: new pull request created to branch release-8.5: #5811.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherry-pick release-8.5

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

kafka: simplify the verify

5 participants