Skip to content

logservice,event: stop capturing tidb_ddl_history#4766

Merged
ti-chi-bot[bot] merged 2 commits intopingcap:masterfrom
wlwilliamx:remove-tidb-ddl-history
Apr 17, 2026
Merged

logservice,event: stop capturing tidb_ddl_history#4766
ti-chi-bot[bot] merged 2 commits intopingcap:masterfrom
wlwilliamx:remove-tidb-ddl-history

Conversation

@wlwilliamx
Copy link
Copy Markdown
Collaborator

@wlwilliamx wlwilliamx commented Apr 8, 2026

What problem does this PR solve?

Issue Number: close #2272

What is changed and how it works?

This PR removes TiCDC's remaining upstream capture path for tidb_ddl_history.

  • stop subscribing to tidb_ddl_history in the schema-store DDL fetcher
  • stop initializing tidb_ddl_history metadata for DDL parsing
  • parse DDL jobs only from tidb_ddl_job
  • accept only JobStateDone jobs from tidb_ddl_job and drop the old JobStateSynced compatibility path
  • add regression tests for DDL span subscription and job parsing behavior

This aligns TiCDC with the TiDB v8.3+ / v8.5 GA behavior targeted by issue #2272, where CREATE TABLE no longer requires tidb_ddl_history capture.

Check List

Tests

  • Unit test
  • Manual test

Questions

Will it cause performance regression or break compatibility?

No performance regression is expected.

This intentionally drops compatibility with older TiDB behavior where CREATE TABLE could be emitted only through tidb_ddl_history. That is the behavior cleanup requested by issue #2272.

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

No.

Release note

TiCDC no longer captures DDLs from `tidb_ddl_history` and relies on `tidb_ddl_job` for accelerated table creation.

Summary by CodeRabbit

  • Refactor
    • Simplified DDL job handling to use only the main DDL job table and removed legacy history-table handling; parsing now focuses on completed jobs for a normalized DDL lifecycle.
  • Tests
    • Added unit tests validating the consolidated span discovery and the updated job-parsing behavior for various DDL job states.

@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 8, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 8, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 8, 2026

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: a6e5c789-dd70-41c5-8c88-e3312cba40d4

📥 Commits

Reviewing files that changed from the base of the PR and between 4e95396 and 7a960ab.

📒 Files selected for processing (1)
  • logservice/schemastore/ddl_job_fetcher_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • logservice/schemastore/ddl_job_fetcher_test.go

📝 Walkthrough

Walkthrough

This PR removes capture and parsing logic for tidb_ddl_history, consolidating DDL job discovery and parsing to tidb_ddl_job only; constants, metadata fields, and conditional history-path logic were deleted and tests updated to assert the new single-span behavior.

Changes

Cohort / File(s) Summary
DDL Job Fetcher
logservice/schemastore/ddl_job_fetcher.go, logservice/schemastore/ddl_job_fetcher_test.go
Dropped metadef dependency and history-table initialization. getAllDDLSpan now returns a single heartbeatpb.TableSpan for common.JobTableID. Added TestGetAllDDLSpan to validate one-span result.
Event Mounter
pkg/common/event/mounter.go, pkg/common/event/mounter_test.go
Removed DDLHistoryTable and JobMetaColumnIDinHistoryTable from DDLTableInfo. Simplified ParseDDLJob/parseJob (removed fromHistoryTable path) to only accept jobs where job.IsDone(). Added tests (TestParseJobFromDDLJob) covering Done/state behaviors.
Span Operations
pkg/common/span_op.go
Removed exported JobHistoryID constant that referenced tidb_ddl_history.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested labels

size/XL

Suggested reviewers

  • lidezhu
  • hongyunyan

Poem

🐰 I hopped through tables, sniffed the past,

Found one that holds the jobs at last.
No history chase, one span to see,
Clean and brisk — a simple key.
🥕 Hop, commit, and off I flee!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: stopping capture of tidb_ddl_history, which is the primary objective of this PR.
Description check ✅ Passed The PR description covers the required sections: issue reference (#2272), detailed explanation of changes, test coverage, compatibility considerations, and a release note.
Linked Issues check ✅ Passed The PR successfully removes tidb_ddl_history capture across multiple components: stops subscribing in DDL fetcher, removes metadata initialization, parses only from tidb_ddl_job, and adds regression tests, fully addressing issue #2272.
Out of Scope Changes check ✅ Passed All changes are directly scoped to removing tidb_ddl_history capture paths from DDL fetching and parsing logic, with supporting test additions, with no extraneous modifications.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.11.4)

Command failed


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 and usage tips.

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 8, 2026
Copy link
Copy Markdown

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

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 simplifies DDL job fetching by removing support for the tidb_ddl_history table, as TiDB v8.3+ now emits all DDL jobs through tidb_ddl_job. The changes refactor the fetcher and mounter to subscribe to and parse only the job table. Feedback indicates that using job.IsDone() is incorrect because it accepts JobStateSynced jobs, which contradicts the goal of only accepting JobStateDone and would cause the new compatibility tests to fail.

Comment on lines +220 to 222
if !job.IsDone() {
return nil, nil
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The use of job.IsDone() here contradicts the PR description ("accept only JobStateDone jobs") and the new test case ignores synced create table compatibility jobs in mounter_test.go. Since IsDone() returns true for JobStateSynced, this implementation will still accept jobs in the Synced state, causing the test to fail. To align with the goal of dropping the JobStateSynced compatibility path, you should check for model.JobStateDone explicitly.

Suggested change
if !job.IsDone() {
return nil, nil
}
if job.State != model.JobStateDone {
return nil, nil
}

@wlwilliamx wlwilliamx marked this pull request as ready for review April 8, 2026 11:02
@ti-chi-bot ti-chi-bot Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2026
@wlwilliamx
Copy link
Copy Markdown
Collaborator Author

/test all

@wlwilliamx
Copy link
Copy Markdown
Collaborator Author

/retest

1 similar comment
@wlwilliamx
Copy link
Copy Markdown
Collaborator Author

/retest

@wlwilliamx
Copy link
Copy Markdown
Collaborator Author

/test pull-cdc-kafka-integration-light

@wlwilliamx
Copy link
Copy Markdown
Collaborator Author

/test pull-cdc-mysql-integration-light

@wlwilliamx
Copy link
Copy Markdown
Collaborator Author

/retest

@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Apr 13, 2026
@wlwilliamx
Copy link
Copy Markdown
Collaborator Author

/retest

@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Apr 14, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 14, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 [hongyunyan,wk989898]

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

@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 14, 2026

[LGTM Timeline notifier]

Timeline:

  • 2026-04-13 04:23:04.482133649 +0000 UTC m=+1362189.687493705: ☑️ agreed by wk989898.
  • 2026-04-14 16:38:03.004131067 +0000 UTC m=+1492688.209491124: ☑️ agreed by hongyunyan.

@wlwilliamx
Copy link
Copy Markdown
Collaborator Author

/retest

@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented Apr 17, 2026

@wlwilliamx: The following tests 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-mysql-integration-heavy 4e95396 link unknown /test pull-cdc-mysql-integration-heavy
pull-unit-test-next-gen 4e95396 link unknown /test pull-unit-test-next-gen

Full PR test history. Your PR dashboard.

Details

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 895bb89 into pingcap:master Apr 17, 2026
12 checks passed
@wlwilliamx wlwilliamx added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Apr 24, 2026
@ti-chi-bot
Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-8.5: #4905.
But this PR has conflicts, please resolve them!

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

Labels

approved lgtm needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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.

remove the capture of the tidb_ddl_history

4 participants