Skip to content

event: assert DDLEvent.NotSync survives Marshal Unmarshal#4420

Open
wlwilliamx wants to merge 1 commit intopingcap:masterfrom
wlwilliamx:codex/ddl-not-sync-marshal-test
Open

event: assert DDLEvent.NotSync survives Marshal Unmarshal#4420
wlwilliamx wants to merge 1 commit intopingcap:masterfrom
wlwilliamx:codex/ddl-not-sync-marshal-test

Conversation

@wlwilliamx
Copy link
Collaborator

@wlwilliamx wlwilliamx commented Mar 10, 2026

What problem does this PR solve?

Issue Number: close #4419

What is changed and how it works?

  • Add a Marshal/Unmarshal round-trip assertion for DDLEvent.NotSync.

Check List

Tests

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

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

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced DDL event handling to properly preserve synchronization state during serialization, ensuring data consistency in event processing.

@ti-chi-bot ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 10, 2026
@ti-chi-bot
Copy link

ti-chi-bot bot commented Mar 10, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign charlescheung96 for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found 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 added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 10, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 10, 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: 39675055-e469-43da-a31f-fa85885c2e83

📥 Commits

Reviewing files that changed from the base of the PR and between f1451ee and 3f1ae66.

📒 Files selected for processing (1)
  • pkg/common/event/ddl_event_test.go

📝 Walkthrough

Walkthrough

Adds a NotSync boolean field to the DDLEvent struct and implements a regression test to verify that this field persists through Marshal/Unmarshal serialization cycles, preventing future encoding changes from inadvertently breaking the skip-forward behavior for DDL events.

Changes

Cohort / File(s) Summary
DDL Event Serialization
pkg/common/event/ddl_event.go, pkg/common/event/ddl_event_test.go
Added NotSync boolean field to DDLEvent struct; updated test to verify field survives round-trip serialization/deserialization.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A field called NotSync finds its place,
In tests that check serialization's trace,
Marshal and unmarshal, round they go,
The rabbit ensures the skip-skip won't flow! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
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 (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding a test assertion for NotSync field serialization/deserialization.
Description check ✅ Passed The description follows the template with Issue Number, problem statement, changes explanation, test checklist, and release notes all properly completed.
Linked Issues check ✅ Passed The PR implements the exact solution requested in issue #4419: a Marshal/Unmarshal round-trip unit test that verifies NotSync field survival through encoding/decoding.
Out of Scope Changes check ✅ Passed All changes are directly related to adding the Marshal/Unmarshal round-trip test for DDLEvent.NotSync as specified in linked issue #4419.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.


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.

@gemini-code-assist
Copy link

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the robustness of DDL event handling by introducing a specific test case. The change verifies that a critical flag, NotSync, maintains its state across serialization and deserialization processes. This ensures that the system reliably determines which DDL events should be dispatched to downstream sinks, preventing potential inconsistencies in data replication.

Highlights

  • DDL Event Serialization: Added a test assertion to ensure that the NotSync field within DDLEvent correctly survives Marshal and Unmarshal operations, which is crucial for dispatchers to properly forward DDL events.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • pkg/common/event/ddl_event_test.go
    • Added a new test case to explicitly set NotSync to true for a DDLEvent.
    • Included an assertion to verify that the NotSync field's value remains consistent after Marshal and Unmarshal operations.
Activity
  • No human activity has occurred on this pull request yet.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@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 adds a test case to ensure that the NotSync field of DDLEvent correctly survives the Marshal/Unmarshal round-trip. This is a valuable addition to confirm the serialization behavior of this critical field, which controls DDL forwarding to downstream sinks. The changes are well-placed within the existing test structure and directly address the stated problem.

@wk989898
Copy link
Collaborator

I think we should use the json:"not_sync" instead of msg:"not_sync".

type DDLEvent struct {
// Version is the version of the DDLEvent struct.
Version int `json:"version"`
DispatcherID common.DispatcherID `json:"-"`
// Type is the type of the DDL.
Type byte `json:"type"`
// SchemaID is from upstream job.SchemaID
SchemaID int64 `json:"schema_id"`
SchemaName string `json:"schema_name"`
TableName string `json:"table_name"`
// the following two fields are just used for RenameTable,
// they are the old schema/table name of the table
ExtraSchemaName string `json:"extra_schema_name"`
ExtraTableName string `json:"extra_table_name"`
Query string `json:"query"`
TableInfo *common.TableInfo `json:"-"`
StartTs uint64 `json:"start_ts"`
FinishedTs uint64 `json:"finished_ts"`
// The seq of the event. It is set by event service.
Seq uint64 `json:"seq"`
// The epoch of the event. It is set by event service.
Epoch uint64 `json:"epoch"`
// MultipleTableInfos holds information for multiple versions of a table.
// The first entry always represents the current table information.
MultipleTableInfos []*common.TableInfo `json:"-"`
BlockedTables *InfluencedTables `json:"blocked_tables"`
// BlockedTableNames is used by downstream adapters to get the names of tables that should block this DDL.
// It is particularly used for querying the execution status of asynchronous DDLs (e.g., `ADD INDEX`)
// that may be running on the table before this DDL.
// This field will be set for most `InfluenceTypeNormal` DDLs, except for those creating new tables/schemas or dropping views.
// It will be empty for other DDLs.
// NOTE: For `RENAME TABLE` / `RENAME TABLES` DDLs, this will be set to the old table names.
// For partition DDLs, this will be the parent table name.
BlockedTableNames []SchemaTableName `json:"blocked_table_names"`
NeedDroppedTables *InfluencedTables `json:"need_dropped_tables"`
NeedAddedTables []Table `json:"need_added_tables"`
// Only set when tables moves between databases
UpdatedSchemas []SchemaIDChange `json:"updated_schemas"`
// DDLs which may change table name:
// Create Table
// Create Tables
// Drop Table
// Rename Table
// Rename Tables
// Drop Schema
// Recover Table
TableNameChange *TableNameChange `json:"table_name_change"`
TiDBOnly bool `json:"tidb_only"`
BDRMode string `json:"bdr_mode"`
Err string `json:"err"`
// Call when event flush is completed
PostTxnFlushed []func() `json:"-"`
// eventSize is the size of the event in bytes. It is set when it's unmarshaled.
eventSize int64 `json:"-"`
// for simple protocol
IsBootstrap bool `msg:"-"`
// NotSync is used to indicate whether the event should be synced to downstream.
// If it is true, sink should not sync this event to downstream.
// It is used for some special DDL events that do not need to be synced,
// but only need to be sent to dispatcher to update some metadata.
// For example, if a `TRUNCATE TABLE` DDL is filtered by event filter,
// we don't need to sync it to downstream, but the DML events of the new truncated table
// should be sent to downstream.
// So we should send the `TRUNCATE TABLE` DDL event to dispatcher,
// to ensure the new truncated table can be handled correctly.
// If the DDL involves multiple tables, this field is not effective.
// The multiple table DDL event will be handled by filtering querys and table infos.
NotSync bool `msg:"not_sync"`
}

@wlwilliamx
Copy link
Collaborator Author

/test all

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

Labels

release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

event: add regression test for DDLEvent.NotSync encoding

2 participants