Skip to content

eventService: Optimize DML processing performance II #5474

Open
asddongmen wants to merge 3 commits into
pingcap:masterfrom
asddongmen:0622-improve-eventBroker
Open

eventService: Optimize DML processing performance II #5474
asddongmen wants to merge 3 commits into
pingcap:masterfrom
asddongmen:0622-improve-eventBroker

Conversation

@asddongmen

@asddongmen asddongmen commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

What problem does this PR solve?

Issue Number: ref #5430

Delete-heavy changefeeds that configure ignore-event = ["delete"] still decoded
ignored delete rows and appended them into DMLEvent before filtering, adding
unnecessary eventService scanner CPU during historical catch-up.

What is changed and how it works?

eventScanner now checks type-only DML filters before row decode for insert/delete
events and caches the result within each transaction. Ignored deletes bypass
DecodeToChunk and DMLEvent.AppendRow. Non-ignored deletes still use the
normal decode, append, and send path.

Check List

Tests

  • Unit test
  • Manual test on 140M row of TTL Deleted row, scan window off:
Scenario Master avg Current avg Result
Ignore delete 338.3s 229.0s 32.3% faster 🔼
No-ignore delete 410.3s 400.7s neutral

Questions

Will it cause performance regression or break compatibility?

No compatibility break expected. No-ignore delete keeps the normal path.

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

No. This is an internal eventService optimization.

Release note

Improve performance for changefeeds that ignore delete events.

Summary by CodeRabbit

  • New Features

    • Enhanced DML event filtering with per-transaction caching for improved performance.
    • Added support for filtering insert, delete, and update events by type without requiring row value decoding.
  • Tests

    • Added benchmark tests to measure filtering performance for DELETE events.
    • Expanded unit test coverage for DML filtering scenarios and event-type-based decisions.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-linked-issue release-note-none Denotes a PR that doesn't merit a release note. labels Jun 22, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown

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 the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 22, 2026
@coderabbitai

coderabbitai Bot commented Jun 22, 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: bc830c6c-94d7-4784-b9ea-774018ec68d6

📥 Commits

Reviewing files that changed from the base of the PR and between 640d3d3 and 3c18b93.

📒 Files selected for processing (5)
  • pkg/eventservice/event_scanner.go
  • pkg/eventservice/event_scanner_benchmark_test.go
  • pkg/eventservice/event_scanner_test.go
  • pkg/filter/filter.go
  • pkg/filter/filter_test.go

📝 Walkthrough

Walkthrough

Adds ShouldIgnoreDMLByEventType to the Filter interface, implementing startTs/table/event-type ignore decisions without decoded row values. dmlProcessor gains a per-transaction RowType ignore cache and uses this method in appendRow to skip decoding and update-splitting when event type alone determines the ignore outcome. New unit tests and benchmarks validate the fast path and cache behavior.

Changes

DML Event-Type Filter Fast Path

Layer / File(s) Summary
Filter interface and ShouldIgnoreDMLByEventType implementation
pkg/filter/filter.go
Adds ShouldIgnoreDMLByEventType to the Filter interface; implements it on filter encapsulating startTs, table, and event-type checks; refactors ShouldIgnoreDML to delegate to it and return early on ignore.
Filter unit test for ShouldIgnoreDMLByEventType
pkg/filter/filter_test.go
Adds TestShouldIgnoreDMLByEventType covering startTs ignore, table inclusion/exclusion, insert-event filter, and delete-value-expr filter across four table metadata instances.
Per-transaction RowType ignore cache in dmlProcessor
pkg/eventservice/event_scanner.go
Adds dmlTypeFilterCache field and constant, resets cache on startTxn, integrates cache helpers into appendRow to skip decode/split for non-update and update rows, and applies separate ignore checks to split insert/delete parts.
dmlProcessor.appendRow unit tests
pkg/eventservice/event_scanner_test.go
Instruments mockMounter with decodeCount, adds countingDMLTypeFilter, and extends TestDMLProcessorAppendRow with subtests verifying delete, update, and split-update ignore paths skip decode and produce correct batch output.
Benchmarks for ignore-delete fast path
pkg/eventservice/event_scanner_benchmark_test.go
Adds BenchmarkDMLProcessorIgnoreDelete and BenchmarkEventScannerIgnoreDelete with fast_path/after_decode sub-benchmarks using a disableDMLTypeFastPathFilter wrapper and a benchmark event getter/iterator.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • pingcap/ticdc#5194: Adds ignore-update-only-columns feature that modifies the same ShouldIgnoreDML path in event_scanner and filter.go which this PR refactors to add the pre-decode event-type fast path.

Suggested labels

size/L, release-note-none

Suggested reviewers

  • flowbehappy
  • hongyunyan
  • 3AceShowHand

Poem

🐇 A cache so quick, it skips the decode,
When event type alone lights the ignore road.
Delete? Update? Filter knows before the parse—
No wasted rows, no unneeded parse.
The rabbit hops past rows at lightning speed! 🚀

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% 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 'eventService: Optimize DML processing performance II' directly addresses the main objective of optimizing DML event processing, which aligns with the changeset's focus on improving performance for delete-heavy changefeeds by filtering before decoding.
Description check ✅ Passed The description comprehensively covers the problem statement (Issue #5430), implementation details, test results with performance metrics, and explicitly addresses compatibility and documentation questions as required by the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

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

@ti-chi-bot ti-chi-bot Bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 22, 2026

@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 introduces a fast-path optimization in the event scanner's DML processor to check if DML events (specifically Insert and Delete) can be ignored based on their event type before performing expensive row decoding. It caches these pre-decode filter results within each transaction and adds corresponding benchmarks and unit tests. The feedback suggests addressing the magic number 3 used for the cache array size by defining a named constant or adding explanatory comments, and explores extending this fast-path optimization to the Update path (both split and non-split updates) to further enhance performance under update-heavy workloads.

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 pkg/eventservice/event_scanner.go Outdated
Comment thread pkg/eventservice/event_scanner.go Outdated
@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-linked-issue labels Jun 23, 2026
@ti-chi-bot ti-chi-bot Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 23, 2026
@asddongmen

Copy link
Copy Markdown
Collaborator Author

/test all

@asddongmen

Copy link
Copy Markdown
Collaborator Author

/retest

@ti-chi-bot

ti-chi-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

@asddongmen: 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-kafka-integration-heavy 3c18b93 link true /test pull-cdc-kafka-integration-heavy

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.

@asddongmen asddongmen marked this pull request as ready for review June 23, 2026 10:59
@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 Jun 23, 2026
Comment thread pkg/filter/filter.go
ShouldIgnoreDML(dmlType common.RowType, preRow, row chunk.Row, tableInfo *common.TableInfo, startTs uint64, ctx DMLFilterContext) (bool, error)
// ShouldIgnoreDMLByEventType returns true only when filters that do not need
// decoded row values can determine the DML should be ignored.
ShouldIgnoreDMLByEventType(dmlType common.RowType, tableInfo *common.TableInfo, startTs uint64) (bool, error)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe ShouldIgnoreDMLBeforeDecode is a better name?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I prefer current name because I think filter shouldn't care if an event is decoded or not.

@lidezhu lidezhu left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM with minor comments

@ti-chi-bot ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 23, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@ti-chi-bot

ti-chi-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-06-23 17:07:11.569268818 +0000 UTC m=+53715.232494331: ☑️ agreed by lidezhu.

@ti-chi-bot ti-chi-bot Bot added the approved label Jun 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants