Skip to content

kafka: franz-go timeout normalize to 10s ~ 15m (#6358) - #6399

Merged
ti-chi-bot[bot] merged 8 commits into
pingcap:release-nextgen-202609from
ti-chi-bot:cherry-pick-6358-to-release-nextgen-202609
Oct 10, 2026
Merged

ti-chi-bot[bot] merged 8 commits into
pingcap:release-nextgen-202609from
ti-chi-bot:cherry-pick-6358-to-release-nextgen-202609

Conversation

@ti-chi-bot

@ti-chi-bot ti-chi-bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

This is an automated cherry-pick of #6358

What problem does this PR solve?

Issue Number: close #6359

What is changed and how it works?

normalize the timeout 10s ~ 15m

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Questions

Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?

Release note

`None`

Summary by CodeRabbit

  • New Features
    • Added Kafka dashboard metrics for batches per request and compression ratios, with average and p99 views.
    • Added clearer context to warnings about missing or invalid Kafka topics.
  • Updates
    • Updated Kafka dashboards to show request rates by broker and latency and throttle time in seconds.
    • Kafka producer read and write timeouts are now constrained to between 10 seconds and 15 minutes; Sarama timeout settings remain unchanged.
    • Kafka request metrics now report aggregate request counts rather than separate success and error counts.

@ti-chi-bot ti-chi-bot added lgtm release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. type/cherry-pick-for-release-nextgen-202609 labels Sep 29, 2026
@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 1dcc57f3-ed58-495a-a364-2324acb37565

📥 Commits

Reviewing files that changed from the base of the PR and between b46dec0 and 17acfa7.

📒 Files selected for processing (12)
  • metrics/grafana/ticdc_new_arch.json
  • metrics/nextgengrafana/ticdc_new_arch_next_gen.json
  • metrics/nextgengrafana/ticdc_new_arch_with_keyspace_name.json
  • pkg/sink/kafka/franz_admin.go
  • pkg/sink/kafka/franz_admin_test.go
  • pkg/sink/kafka/franz_config_test.go
  • pkg/sink/kafka/franz_metrics.go
  • pkg/sink/kafka/franz_metrics_hook.go
  • pkg/sink/kafka/franz_metrics_hook_test.go
  • pkg/sink/kafka/metrics.go
  • pkg/sink/kafka/options.go
  • pkg/sink/kafka/options_test.go

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The pull request updates Kafka producer metrics and three Grafana dashboards, normalizes Franz client timeouts, and adds changefeed context to warnings for ignored topic metadata errors.

Changes

Kafka producer metrics

Layer / File(s) Summary
Producer metric instrumentation
pkg/sink/kafka/franz_metrics.go, pkg/sink/kafka/franz_metrics_hook.go, pkg/sink/kafka/metrics.go, pkg/sink/kafka/franz_metrics_hook_test.go
Metric declarations and hook observations use updated request metrics and add batches-per-request and compression-ratio histograms. Registration and tests reflect these changes.
Kafka dashboard panels
metrics/grafana/ticdc_new_arch.json, metrics/nextgengrafana/ticdc_new_arch_next_gen.json, metrics/nextgengrafana/ticdc_new_arch_with_keyspace_name.json
The dashboards add a batches-per-request panel and update producer queries, grouping, units, and layout.

Franz timeout normalization

Layer / File(s) Summary
Timeout normalization and validation
pkg/sink/kafka/options.go, pkg/sink/kafka/options_test.go, pkg/sink/kafka/franz_config_test.go
Franz read and write timeouts are clamped to 10 seconds through 15 minutes during options application. Tests check the bounds and verify Sarama timeout values remain unchanged.

Topic metadata warnings

Layer / File(s) Summary
Ignored-topic warning context
pkg/sink/kafka/franz_admin.go, pkg/sink/kafka/franz_admin_test.go
Warnings for ignored topic metadata errors include keyspace, changefeed, topic, and error details. The test passes a changefeed ID to the helper.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: 3aceshowhand

Merge Risk: ⚪ Minimal · up to 17acf

The Kafka metrics, timeout, and warning changes have no established merge-blocking issue. Merge after normal checks.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 17acf

Kafka producer monitoring will no longer distinguish request failures in its existing per-broker metrics. The revised dashboards follow the new metrics, but other monitoring may need migration. Timeout changes also affect explicitly configured Franz clients; no new authorization path was identified.

Retained concerns

  • Medium · reliability · observed: The revised producer metric contract removes per-broker request-result and response-error classification. The updated request-rate dashboard likewise shows requests without a result breakdown, so monitoring that relies on those categories will lose that failure signal during migration. Whether another deployed alert supplies equivalent detection is unknown.
Security review details

Security Blast Radius

  • inferred — The direct exposure is the configured Kafka changefeed’s Franz producer behavior and its broker- and changefeed-labelled telemetry. Evidence does not establish who may change the sink URI or whether an external actor can control broker IDs.

Trust Boundaries and Controls

  • observed — URI-supplied timeout durations must be positive. Only Franz read and write timeouts receive the new bounds; dial timeout and Sarama timeouts do not take that normalization branch.

Resilience and Maintainability Implications

  • observed — Construction failure and normal Franz factory close invoke metric cleanup; cleanup includes the newly added histogram series and removes broker-labelled series by changefeed labels. Callback behavior racing with close was not established.

Hardening Proposals

  • proposed — Before retiring result-labelled producer metrics, verify that deployed failure alerts have an equivalent signal and coordinate their migration with the producer and dashboard rollout.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Timeout normalization in pkg/sink/kafka/options.go and its tests are in scope for the historical issue. The changes to pkg/sink/kafka/franz_metrics.go, pkg/sink/kafka/franz_metrics_hook.go, `pkg… Remove the unrelated metrics, dashboard, and logging changes from this pull request, or link them to a separate active coding objective.
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 9 files. (3 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The description identifies the linked issue, summarizes timeout normalization, and includes the required template sections. The test checklist is not marked, and both Questions fields are unanswered, …
Title check ✅ Passed The title clearly identifies the primary change: normalizing franz-go Kafka timeouts to the 10-second to 15-minute range.
Linked Issues check ✅ Passed Issue #6359 is closed and supplies historical context only. No active directly linked issue remains, so this pull request has no linked-issue coding requirements.
Full details: Out of Scope Changes check

Explanation

Timeout normalization in pkg/sink/kafka/options.go and its tests are in scope for the historical issue. The changes to pkg/sink/kafka/franz_metrics.go, pkg/sink/kafka/franz_metrics_hook.go, pkg/sink/kafka/metrics.go, the three Grafana dashboards, and pkg/sink/kafka/franz_admin.go do not modify timeout handling or test timeout behavior. These metrics, dashboard, and logging changes are unrelated to the stated timeout objective.

Full details: Docstring Coverage

Explanation

Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 9 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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

A rabbit checks the panels glow,
New batch counts begin to show.
The timeouts stay within their bounds,
Topic warnings gain clearer sounds.
I twitch my nose and hop along!

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

@ti-chi-bot ti-chi-bot Bot added the approved label Sep 29, 2026
@3AceShowHand

Copy link
Copy Markdown
Collaborator

/test all

@ti-chi-bot ti-chi-bot Bot added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels Oct 9, 2026
@ti-chi-bot

ti-chi-bot Bot commented Oct 9, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asddongmen, lidezhu

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:

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

/test pull-build-next-gen

@3AceShowHand

Copy link
Copy Markdown
Collaborator

/test next-gen

3 similar comments
@3AceShowHand

Copy link
Copy Markdown
Collaborator

/test next-gen

@3AceShowHand

Copy link
Copy Markdown
Collaborator

/test next-gen

@3AceShowHand

Copy link
Copy Markdown
Collaborator

/test next-gen

@3AceShowHand

Copy link
Copy Markdown
Collaborator

/retest

@3AceShowHand

Copy link
Copy Markdown
Collaborator

/retest

@ti-chi-bot
ti-chi-bot Bot merged commit 6762eee into pingcap:release-nextgen-202609 Oct 10, 2026
37 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved cherry-pick-approved Cherry pick PR approved by release team. lgtm release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. type/cherry-pick-for-release-nextgen-202609

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants