Skip to content

upstream: support ticdc topology info in tidb dashboard #1209

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

Merged
merged 6 commits into from
May 28, 2025

Conversation

wlwilliamx
Copy link
Collaborator

@wlwilliamx wlwilliamx commented Apr 7, 2025

What problem does this PR solve?

Issue Number: close #1115

What is changed and how it works?

This PR adds support for showing TiCDC cluster node information in the TiDB Dashboard. It works by writing each TiCDC node’s topology info into etcd under the key /topology/ticdc/{clusterID}/{ip:port}. The data includes basic node metadata and is registered with a lease so it gets cleaned up automatically if the node goes down.

The TiDB Dashboard can read from the /topology/ticdc/ prefix, parse the values, and display the list of active TiCDC nodes along with their details. This helps users get a better view of the CDC cluster status directly from the dashboard without needing to query it separately.

Related code in tidb-dashboard repo: https://github.com/pingcap/tidb-dashboard/blob/release-9.0-beta.1/pkg/utils/topology/ticdc.go#L22.

Check List

Tests

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

CleanShot 2025-04-07 at 12 15 33@2x
CleanShot 2025-04-07 at 12 16 15@2x
CleanShot 2025-04-07 at 12 16 36@2x

Questions

Will it cause performance regression or break compatibility?

No.

Do you need to update user documentation, design documentation or monitoring documentation?

No.

Release note

None

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-triage-completed 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. labels Apr 7, 2025
@wlwilliamx wlwilliamx requested a review from Copilot April 7, 2025 06:16
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

pkg/upstream/upstream.go:358

  • The use of 'clientV3.WithLease' here may not be compatible with the updated etcd.Client interface. Consider using the corresponding lease option provided by the new etcd client wrapper to ensure consistency.
_, err = up.etcdCli.Put(ctx, key, string(value), clientV3.WithLease(up.session.Lease()))

pkg/upstream/manager.go:31

  • The comment refers to 'CaptureTopologyCfg' while the actual type is named 'NodeTopologyCfg'. Consider updating the comment to match the type name for clarity.
// CaptureTopologyCfg stores the information of the capture topology.

@wlwilliamx
Copy link
Collaborator Author

/check-issue-triage-complete

@wk989898
Copy link
Collaborator

wk989898 commented Apr 7, 2025

Does it conflict with the old TiCDC?

}

// fetchTiDBTopology parses the TiDB topology from etcd.
func fetchTiDBTopology(ctx context.Context, etcdClient *clientv3.Client) ([]tidbInstance, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the ticdc topology depend on it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, no. But we will need it later.

@ti-chi-bot ti-chi-bot bot added the lgtm label May 21, 2025
Copy link

ti-chi-bot bot commented May 21, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flowbehappy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

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

Copy link

ti-chi-bot bot commented May 21, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-05-21 09:11:53.736650866 +0000 UTC m=+166550.837818088: ☑️ agreed by flowbehappy.

@ti-chi-bot ti-chi-bot bot added the approved label May 21, 2025
@wlwilliamx
Copy link
Collaborator Author

Does it conflict with the old TiCDC?

No.

@wlwilliamx wlwilliamx requested review from wk989898 and Copilot May 21, 2025 09:42
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for registering TiCDC node topology in etcd so the TiDB Dashboard can display cluster node information.

  • Introduce an upstream manager to register node topology info with etcd using a lease.
  • Update server initialization to wire up the upstream manager.
  • Add a new etcd client wrapper helper and a helper for fetching TiDB topology.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
server/server.go Initialize upstream.Manager with NodeTopologyCfg and register default upstream.
pkg/upstream/upstream.go Add registerTopologyInfo to publish TiCDC node info under /topology/ticdc.
pkg/upstream/topo.go New helper to fetch TiDB topology from etcd for reference.
pkg/upstream/manager.go Pass NodeTopologyCfg into init functions and update signatures.
pkg/etcd/client.go Add NewWrappedClient to simplify creating a wrapped etcd client.
Comments suppressed due to low confidence (2)

pkg/upstream/upstream.go:342

  • There are no tests covering registerTopologyInfo; consider adding unit tests to verify the etcd key format, TTL behavior, and error cases.
func (up *Upstream) registerTopologyInfo(ctx context.Context, cfg *NodeTopologyCfg) error {

pkg/upstream/upstream.go:358

  • Use the correct alias for the imported etcd client: change clientV3.WithLease to clientv3.WithLease to match the clientv3 import and ensure compilation.
_, err = up.etcdCli.Put(ctx, key, string(value), clientV3.WithLease(up.session.Lease()))

@asddongmen
Copy link
Collaborator

/retest

1 similar comment
@wlwilliamx
Copy link
Collaborator Author

/retest

@wlwilliamx
Copy link
Collaborator Author

/test pull-cdc-mysql-integration-light

@wlwilliamx
Copy link
Collaborator Author

/test pull-cdc-mysql-integration-heavy

@wlwilliamx
Copy link
Collaborator Author

/test pull-cdc-kafka-integration-light

@wlwilliamx
Copy link
Collaborator Author

/test pull-cdc-mysql-integration-heavy

@purelind
Copy link

/test pull-cdc-mysql-integration-heavy

1 similar comment
@wlwilliamx
Copy link
Collaborator Author

/test pull-cdc-mysql-integration-heavy

@wlwilliamx
Copy link
Collaborator Author

/test pull-cdc-mysql-integration-light

@wlwilliamx
Copy link
Collaborator Author

/test pull-cdc-mysql-integration-heavy

@wlwilliamx
Copy link
Collaborator Author

/test pull-cdc-kafka-integration-heavy

Copy link

ti-chi-bot bot commented May 28, 2025

@wlwilliamx: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cdc-pulsar-integration-light db9a56b link false /test pull-cdc-pulsar-integration-light

Full PR test history. Your PR dashboard.

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@ti-chi-bot ti-chi-bot bot merged commit e8c122e into pingcap:master May 28, 2025
14 of 15 checks passed
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.

TiCDC new arch is not compatible with dashboard
5 participants