Skip to content

mysql,sqlmodel: support table route in mysql sink#5006

Open
3AceShowHand wants to merge 18 commits into
masterfrom
table-route-pr6-mysql-sink
Open

mysql,sqlmodel: support table route in mysql sink#5006
3AceShowHand wants to merge 18 commits into
masterfrom
table-route-pr6-mysql-sink

Conversation

@3AceShowHand
Copy link
Copy Markdown
Collaborator

@3AceShowHand 3AceShowHand commented May 7, 2026

What problem does this PR solve?

Issue Number: close #4818

This PR makes the MySQL sink honor table route rules when generating and
executing downstream SQL. Before this change, parts of the MySQL sink still used
source schema/table names directly, so a routed changefeed could generate DML or
execute DDL against the source database/table instead of the routed downstream
target.

What is changed and how it works?

  • Use routed schema names when executing DDL in the MySQL sink.

    • needSwitchDB checks DDLEvent.GetTargetSchemaName().
    • execDDL runs USE <target schema> when the DDL needs a default database.
  • Use routed table names when generating MySQL DML SQL.

    • MySQL insert/delete/update and active-active upsert SQL use the target table
      name stored in TableInfo.
    • sqlmodel single-row and multi-row insert/delete/update builders use
      QuoteTargetString(), so the same row model can generate SQL for routed
      targets.
  • Keep routed metadata usable across DDL and DML paths.

    • DDL rewrite fills the DDL event default schema before applying routing, so
      unqualified table names in DDL can be routed correctly.
    • Routed TableInfo clones initialize their private SQL fields before being
      used by downstream sinks.
    • The event collector avoids replacing a dispatcher's cached table info with
      unrelated DDL table info, while still advancing the delivered table info
      version for DDLs sent to that dispatcher.
  • Add a MySQL-only table route integration test.

    • The test routes source_db.* to target_db.*_routed and
      source_extra_db.* to target_extra_db.*_routed.
    • It covers DML, create/alter/drop/rename/truncate table, create/drop view,
      partition DDL, cross-database DDL, and drop database.
    • It validates routed data with sync-diff route rules and checks that dropped
      routed databases disappear downstream.

Check List

Tests

  • Unit test
  • Integration test

Questions

Will it cause performance regression or break compatibility?

No compatibility break is expected. Without table route rules, target schema and
table names fall back to the original source names.

No meaningful performance regression is expected. The MySQL sink uses target
names already carried by routed events/table info instead of applying routing in
the DML hot path.

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

No. This PR wires existing table route behavior into the MySQL sink and adds
integration coverage; it does not introduce new user-facing configuration or
monitoring items.

Release note

Support table route for the MySQL sink.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels May 7, 2026
@ti-chi-bot
Copy link
Copy Markdown

ti-chi-bot Bot commented May 7, 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 tenfyzhong 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

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 7, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR makes the MySQL sink routing-aware: DDL execution and needSwitchDB use routed target schema names; DML builders and RowChange emit target-quoted table identifiers; the DDL rewriter fills default schema before routing; TableInfo init and dispatcher caching were updated; unit and integration tests validate routed behavior.

Changes

Table Routing for MySQL Sink

Layer / File(s) Summary
DDL Schema Routing
pkg/sink/mysql/helper.go, pkg/sink/mysql/mysql_writer_ddl.go
needSwitchDB checks event.GetTargetSchemaName() instead of source schema; execDDL issues USE with event.GetTargetSchemaName().
DDL Query Rewriting
downstreamadapter/routing/ddl_query_rewriter.go, downstreamadapter/routing/router_apply_test.go
Per-statement rewriter accepts defaultSchema; missing schema names in extracted tables are filled before routing and AST rewriting.
DML Row Change Base
pkg/sink/sqlmodel/row_change.go
TargetTableID(), genDeleteSQL(), and genUpdateSQL() now use QuoteTargetString() for target table identifiers.
DML SQL Quoting (Single-Row)
pkg/sink/mysql/sql_builder.go
buildDelete and buildActiveActiveUpsertSQL quote target tables using QuoteTargetString().
DML SQL Quoting (Multi-Row)
pkg/sink/sqlmodel/multi_row.go, pkg/sink/sqlmodel/multi_row_v1.go
GenInsertSQL, genDeleteSQLV2, genUpdateSQLV2, and V1 variants render target table via QuoteTargetString(); genUpdateSQLV1 return type updated to []any.
Event Collection
downstreamadapter/eventcollector/dispatcher_stat.go, downstreamadapter/eventcollector/dispatcher_stat_test.go
Table-info/version update moved into updateTableInfoByDDL; caching of ddl.TableInfo is gated on dispatcher TableSpan/table ID; tests updated for mockDispatcher.tableSpan.
DDL Execution Test
pkg/sink/mysql/mysql_writer_test.go
TestMysqlWriterExecDDLUsesRoutedSchemaName asserts execDDL issues USE \target_db`;, SET TIMESTAMP = DEFAULT`, and executes the routed DDL query via sqlmock.
DDL Router Tests
downstreamadapter/routing/router_supported_ddl_test.go
TestApplyToDDLEventRoutesDDLEventMetadata verifies routing rewrites query/table names, preserves origin metadata, and deep-copies TableInfo and PostTxnFlushed slices.
DML Unit Tests
pkg/sink/mysql/sql_builder_test.go, pkg/sink/sqlmodel/multi_row_test.go, pkg/sink/sqlmodel/row_change_test.go, pkg/sink/mysql/mysql_writer_dml_active_active_test.go
New/updated tests verify builders and RowChange generate statements targeting routed tables and exclude source table references across single-row, multi-row, and V1/V2 strategies.
Integration Test Configuration
tests/integration_tests/run_light_it_in_ci.sh, tests/integration_tests/table_route/conf/changefeed.toml, tests/integration_tests/table_route/conf/diff_config.toml
CI group G07 includes table_route; changefeed/diff configs map source_db.*/source_extra_db.* to routed target_db/target_extra_db targets for verification.
Integration Test Workload
tests/integration_tests/table_route/data/test.sql, tests/integration_tests/table_route/run.sh
SQL fixture exercises comprehensive DDL/DML scenarios (inserts, updates, renames, views, partitioning, truncation); bash runner starts TiDB/CDC, applies fixture, verifies routed results and diffs.
TableInfo Init & Event Init Cleanup
pkg/common/table_info.go, pkg/common/event/*.go, pkg/common/event/*_test.go
InitPrivateFields is invoked earlier for NewTableInfo/CloneWithRouting/Unmarshal; removed deferred InitPrivateFields from several event decode/assemble paths and adjusted tests accordingly.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • pingcap/ticdc#4658: Both PRs touch the same event/table-name routing surface—adding/using routed target schema/table fields and APIs (e.g., DDLEvent target getters, NewRoutedDDLEvent, TableName.Target*/QuoteTargetString/CloneWithRouting).
  • pingcap/ticdc#4270: Overlapping changes to execDDL and DDL preprocessing.
  • pingcap/ticdc#4659: Related routing core (router, ApplyToDDLEvent) that this sink integration builds upon.

Suggested labels

lgtm, approved

Suggested reviewers

  • hongyunyan
  • wk989898
  • lidezhu

Poem

🐰 I hopped through schemas with a cheerful grin,
Swapped source for target, let routing begin,
Quoted the tables with tidy delight,
Tests clap their paws through day and night,
The routed path now runs snug and trim.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 13.33% 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 'mysql,sqlmodel: support table route in mysql sink' clearly and concisely describes the main change: adding table route support to the MySQL sink components.
Linked Issues check ✅ Passed The PR successfully implements table route support in the MySQL sink as required by issue #4818, with comprehensive code changes, unit tests, integration tests, and a complete test workflow.
Out of Scope Changes check ✅ Passed All changes are directly related to enabling table route functionality in the MySQL sink; no unrelated modifications were introduced outside the stated objective.
Description check ✅ Passed PR description follows the template structure with all required sections completed: issue reference, detailed explanation of changes, test checklist, compatibility assessment, and release note.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch table-route-pr6-mysql-sink

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 7, 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 ensures that table routing is correctly applied to MySQL sinks for both DML and DDL operations. It updates the SQL generation logic across several files to use target schema and table names (via GetTargetSchemaName and QuoteTargetString) instead of source names. Additionally, it introduces a new integration test suite and several unit tests to verify that routed tables are correctly handled during replication. I have no feedback to provide as there were no review comments.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/sink/sqlmodel/multi_row_test.go`:
- Around line 44-103: The test TestGenMultiRowSQLUsesRoutedTargetTable is
missing t.Parallel(); add t.Parallel() as the first statement in that function
so it runs concurrently with the other tests in multi_row_test.go, keeping test
behavior unchanged otherwise; locate the TestGenMultiRowSQLUsesRoutedTargetTable
function and insert the call at the top.

In `@tests/integration_tests/table_route/run.sh`:
- Around line 6-84: The script suffers from unquoted variable expansions which
can cause word-splitting/globbing issues; go through the run() function and
quote all shell variable expansions used in commands and args (e.g. "$WORK_DIR",
"$OUT_DIR", "$TEST_NAME", "$CUR", "$DOWN_TIDB_HOST", "$DOWN_TIDB_PORT",
"$UP_TIDB_HOST", "$UP_TIDB_PORT", "$CDC_BINARY", "$SINK_URI", "$KEYSPACE_NAME")
— update calls like rm -rf $WORK_DIR, mkdir -p $WORK_DIR, start_tidb_cluster
--workdir $WORK_DIR,
run_sql/run_sql_file/check_table_exists/check_table_not_exists/run_cdc_server/cdc_cli_changefeed
invocations and any literal path concatenations (e.g. $CUR/conf/changefeed.toml,
$CUR/data/prepare.sql) to use double-quoted expansions so arguments are passed
safely.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 13e6b16a-4f9e-4de3-943d-e295d7a16f94

📥 Commits

Reviewing files that changed from the base of the PR and between 29a8576 and 72dddae.

📒 Files selected for processing (17)
  • pkg/sink/mysql/helper.go
  • pkg/sink/mysql/mysql_writer_ddl.go
  • pkg/sink/mysql/mysql_writer_dml_active_active_test.go
  • pkg/sink/mysql/mysql_writer_test.go
  • pkg/sink/mysql/sql_builder.go
  • pkg/sink/mysql/sql_builder_test.go
  • pkg/sink/sqlmodel/multi_row.go
  • pkg/sink/sqlmodel/multi_row_test.go
  • pkg/sink/sqlmodel/multi_row_v1.go
  • pkg/sink/sqlmodel/row_change.go
  • pkg/sink/sqlmodel/row_change_test.go
  • tests/integration_tests/run_light_it_in_ci.sh
  • tests/integration_tests/table_route/README.md
  • tests/integration_tests/table_route/conf/changefeed.toml
  • tests/integration_tests/table_route/data/prepare.sql
  • tests/integration_tests/table_route/data/test.sql
  • tests/integration_tests/table_route/run.sh

Comment thread pkg/sink/sqlmodel/multi_row_test.go
Comment thread tests/integration_tests/table_route/run.sh Outdated
@3AceShowHand 3AceShowHand force-pushed the table-route-pr6-mysql-sink branch from 72dddae to 8729a9d Compare May 7, 2026 09:27
@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 May 7, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
tests/integration_tests/table_route/run.sh (1)

6-50: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Quote variable expansions in command arguments to avoid word-splitting/globbing bugs.

Unquoted expansions are still present across the run path (source, path concatenations, host/port args, workdir args, trap/check_logs). This can break when values contain spaces or glob chars.

Suggested minimal hardening patch
-source $CUR/../_utils/test_prepare
-WORK_DIR=$OUT_DIR/$TEST_NAME
+source "$CUR/../_utils/test_prepare"
+WORK_DIR="$OUT_DIR/$TEST_NAME"
@@
-	rm -rf $WORK_DIR && mkdir -p $WORK_DIR
+	rm -rf "$WORK_DIR" && mkdir -p "$WORK_DIR"
@@
-	start_tidb_cluster --workdir $WORK_DIR
+	start_tidb_cluster --workdir "$WORK_DIR"
@@
-	run_cdc_server --workdir $WORK_DIR --binary $CDC_BINARY --cluster-id "$KEYSPACE_NAME"
+	run_cdc_server --workdir "$WORK_DIR" --binary "$CDC_BINARY" --cluster-id "$KEYSPACE_NAME"
@@
-	run_sql_file $CUR/data/test.sql ${UP_TIDB_HOST} ${UP_TIDB_PORT}
+	run_sql_file "$CUR/data/test.sql" "${UP_TIDB_HOST}" "${UP_TIDB_PORT}"
@@
-	check_table_exists target_db.finish_mark_routed ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT} 90
-	check_sync_diff $WORK_DIR $CUR/conf/diff_config.toml 120
+	check_table_exists target_db.finish_mark_routed "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}" 90
+	check_sync_diff "$WORK_DIR" "$CUR/conf/diff_config.toml" 120
@@
-	check_table_not_exists source_db.users ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT}
-	check_table_not_exists source_db.orders ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT}
-	check_table_not_exists target_db.temp_table_routed ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT}
-	check_table_not_exists target_db.multi_rename_a_routed ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT}
-	check_table_not_exists target_db.multi_rename_b_routed ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT}
-	check_table_not_exists target_db.to_be_dropped_routed ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT}
-	run_sql "SHOW CREATE VIEW target_db.user_order_view_routed" ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT}
+	check_table_not_exists source_db.users "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}"
+	check_table_not_exists source_db.orders "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}"
+	check_table_not_exists target_db.temp_table_routed "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}"
+	check_table_not_exists target_db.multi_rename_a_routed "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}"
+	check_table_not_exists target_db.multi_rename_b_routed "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}"
+	check_table_not_exists target_db.to_be_dropped_routed "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}"
+	run_sql "SHOW CREATE VIEW target_db.user_order_view_routed" "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}"
@@
-	check_table_not_exists target_db.transient_view_routed ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT}
+	check_table_not_exists target_db.transient_view_routed "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}"
@@
-	run_sql "DROP DATABASE source_db" ${UP_TIDB_HOST} ${UP_TIDB_PORT}
-	check_db_not_exists target_db ${DOWN_TIDB_HOST} ${DOWN_TIDB_PORT} 90
+	run_sql "DROP DATABASE source_db" "${UP_TIDB_HOST}" "${UP_TIDB_PORT}"
+	check_db_not_exists target_db "${DOWN_TIDB_HOST}" "${DOWN_TIDB_PORT}" 90
@@
-trap 'stop_test $WORK_DIR' EXIT
+trap 'stop_test "$WORK_DIR"' EXIT
@@
-check_logs $WORK_DIR
+check_logs "$WORK_DIR"
#!/bin/bash
# Verify remaining unquoted variable expansions in this script (read-only).
# Expected result after fix: no SC2086 findings for this file.
shellcheck -x tests/integration_tests/table_route/run.sh
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration_tests/table_route/run.sh` around lines 6 - 50, The script
leaves many unquoted variable expansions which can cause word-splitting/globbing
bugs; update all command arguments and path concatenations to quote variables
(e.g., "$CUR", "$OUT_DIR", "$TEST_NAME", "$WORK_DIR", "$CDC_BINARY",
"$SINK_TYPE", "$KEYSPACE_NAME", "$DOWN_TIDB_HOST", "$DOWN_TIDB_PORT",
"$UP_TIDB_HOST", "$UP_TIDB_PORT", "$SINK_URI") used in run(), the trap/stop_test
call, start_tidb_cluster, run_cdc_server, cdc_cli_changefeed create,
run_sql_file, run_sql, check_* and check_logs so every invocation uses quoted
expansions like "$VAR" to avoid SC2086-style issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@tests/integration_tests/table_route/run.sh`:
- Around line 6-50: The script leaves many unquoted variable expansions which
can cause word-splitting/globbing bugs; update all command arguments and path
concatenations to quote variables (e.g., "$CUR", "$OUT_DIR", "$TEST_NAME",
"$WORK_DIR", "$CDC_BINARY", "$SINK_TYPE", "$KEYSPACE_NAME", "$DOWN_TIDB_HOST",
"$DOWN_TIDB_PORT", "$UP_TIDB_HOST", "$UP_TIDB_PORT", "$SINK_URI") used in run(),
the trap/stop_test call, start_tidb_cluster, run_cdc_server, cdc_cli_changefeed
create, run_sql_file, run_sql, check_* and check_logs so every invocation uses
quoted expansions like "$VAR" to avoid SC2086-style issues.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e6191306-fa31-4e36-ab31-2c57e90b2dfe

📥 Commits

Reviewing files that changed from the base of the PR and between 72dddae and 41025ce.

📒 Files selected for processing (20)
  • downstreamadapter/routing/router_supported_ddl_test.go
  • pkg/common/event/ddl_event.go
  • pkg/common/event/ddl_event_test.go
  • pkg/sink/mysql/helper.go
  • pkg/sink/mysql/mysql_writer_ddl.go
  • pkg/sink/mysql/mysql_writer_dml_active_active_test.go
  • pkg/sink/mysql/mysql_writer_test.go
  • pkg/sink/mysql/sql_builder.go
  • pkg/sink/mysql/sql_builder_test.go
  • pkg/sink/sqlmodel/multi_row.go
  • pkg/sink/sqlmodel/multi_row_test.go
  • pkg/sink/sqlmodel/multi_row_v1.go
  • pkg/sink/sqlmodel/row_change.go
  • pkg/sink/sqlmodel/row_change_test.go
  • tests/integration_tests/run_light_it_in_ci.sh
  • tests/integration_tests/table_route/README.md
  • tests/integration_tests/table_route/conf/changefeed.toml
  • tests/integration_tests/table_route/conf/diff_config.toml
  • tests/integration_tests/table_route/data/test.sql
  • tests/integration_tests/table_route/run.sh
💤 Files with no reviewable changes (1)
  • pkg/common/event/ddl_event_test.go
✅ Files skipped from review due to trivial changes (2)
  • tests/integration_tests/table_route/conf/diff_config.toml
  • tests/integration_tests/table_route/conf/changefeed.toml
🚧 Files skipped from review as they are similar to previous changes (10)
  • pkg/sink/mysql/mysql_writer_ddl.go
  • tests/integration_tests/run_light_it_in_ci.sh
  • pkg/sink/mysql/helper.go
  • tests/integration_tests/table_route/README.md
  • pkg/sink/mysql/sql_builder_test.go
  • pkg/sink/mysql/mysql_writer_dml_active_active_test.go
  • tests/integration_tests/table_route/data/test.sql
  • pkg/sink/mysql/mysql_writer_test.go
  • pkg/sink/sqlmodel/multi_row_test.go
  • pkg/sink/sqlmodel/row_change_test.go

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/retest

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
downstreamadapter/eventcollector/dispatcher_stat.go (1)

463-472: 💤 Low value

Optional: hoist the cached-id lookup for clarity.

Minor readability nit only — the two-step expectedTableID computation is fine, but reading "use span id, then maybe override from cache" backwards is slightly awkward. A small refactor could fold the cache into the conditional directly:

♻️ Suggested refactor (no behavior change)
-	expectedTableID := tableSpan.TableID
-	current := d.tableInfo.Load()
-	if current != nil {
-		expectedTableID = current.(*common.TableInfo).TableName.TableID
-	}
-	if ddl.TableInfo.TableName.TableID != expectedTableID {
-		return
-	}
+	expectedTableID := tableSpan.TableID
+	if cached, ok := d.tableInfo.Load().(*common.TableInfo); ok && cached != nil {
+		// For partition tables, span TableID is physical; cache holds the logical id.
+		expectedTableID = cached.TableName.TableID
+	}
+	if ddl.TableInfo.TableName.TableID != expectedTableID {
+		return
+	}

Feel free to skip if you prefer the current form.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@downstreamadapter/eventcollector/dispatcher_stat.go` around lines 463 - 472,
Hoist the cached-table-id lookup for clarity by first loading d.tableInfo.Load()
into a local variable (e.g., cached := d.tableInfo.Load()), then set
expectedTableID to the cached id when cached != nil (casting to
*common.TableInfo and reading TableName.TableID), otherwise fall back to
tableSpan.TableID; finally compare ddl.TableInfo.TableName.TableID to
expectedTableID as before. Update references to expectedTableID,
tableSpan.TableID, d.tableInfo.Load(), and common.TableInfo to reflect this
reordered logic without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@downstreamadapter/eventcollector/dispatcher_stat.go`:
- Around line 463-472: Hoist the cached-table-id lookup for clarity by first
loading d.tableInfo.Load() into a local variable (e.g., cached :=
d.tableInfo.Load()), then set expectedTableID to the cached id when cached !=
nil (casting to *common.TableInfo and reading TableName.TableID), otherwise fall
back to tableSpan.TableID; finally compare ddl.TableInfo.TableName.TableID to
expectedTableID as before. Update references to expectedTableID,
tableSpan.TableID, d.tableInfo.Load(), and common.TableInfo to reflect this
reordered logic without changing behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6b15bd65-7674-4f51-be23-8552570a7506

📥 Commits

Reviewing files that changed from the base of the PR and between 41025ce and b90a0aa.

📒 Files selected for processing (10)
  • downstreamadapter/eventcollector/dispatcher_stat.go
  • downstreamadapter/eventcollector/dispatcher_stat_test.go
  • downstreamadapter/routing/ddl_query_rewriter.go
  • downstreamadapter/routing/router_apply_test.go
  • downstreamadapter/routing/router_supported_ddl_test.go
  • pkg/sink/sqlmodel/multi_row_test.go
  • tests/integration_tests/table_route/conf/changefeed.toml
  • tests/integration_tests/table_route/conf/diff_config.toml
  • tests/integration_tests/table_route/data/test.sql
  • tests/integration_tests/table_route/run.sh
🚧 Files skipped from review as they are similar to previous changes (3)
  • tests/integration_tests/table_route/conf/diff_config.toml
  • pkg/sink/sqlmodel/multi_row_test.go
  • tests/integration_tests/table_route/run.sh

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pkg/common/event/dml_event.go (1)

295-301: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Unresolved crash path: partition DDL + routing version mismatch will log.Panic in production.

The TODO correctly identifies the scenario: when a partition DDL (e.g. TRUNCATE PARTITION, EXCHANGE PARTITION) races with the dispatcher's cached routed TableInfo, originVersion != routedVersion can be legitimately true, and log.Panic at line 298 will terminate the process.

This PR enables table routing in the MySQL sink; any changefeed that combines routing with partition DDLs on the replicated tables would hit this path. The same version-mismatch guard in the remote-rows path (lines 315–323) has no TODO and would face the same risk.

Before this change ships, the expected behavior for this case needs to be defined — either:

  1. Accept that the stale local event is compatible with the new schema (e.g., columns didn't change, only partition layout did) and proceed without panicking; or
  2. Discard the event and signal a re-sync.

Do you want me to draft a fix that differentiates DDL-driven version bumps (where the column schema is still compatible) from genuine schema incompatibilities, so only the latter panics?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/common/event/dml_event.go` around lines 295 - 301, The log.Panic on a
routed TableInfo version mismatch must be replaced with compatibility-aware
handling: in the block where originVersion != routedVersion (around the routed
table info check in dml_event.go), do not panic unconditionally—compare the
origin TableInfo and routed TableInfo column schemas (implement or call a helper
like areColumnSchemasCompatible(originTableInfo, routedTableInfo)); if schemas
are compatible (e.g., only partition/metadata changed) proceed processing the
local row using the routed TableInfo and log a warning; if incompatible, drop
the event and surface a sync-needed error (or keep the existing panic only for
confirmed incompatible schema changes) so consumers can trigger a re-sync. Also
align behavior with the remote-rows path (the guard around lines 315–323) to
ensure consistent handling of version mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@pkg/common/event/dml_event.go`:
- Around line 295-301: The log.Panic on a routed TableInfo version mismatch must
be replaced with compatibility-aware handling: in the block where originVersion
!= routedVersion (around the routed table info check in dml_event.go), do not
panic unconditionally—compare the origin TableInfo and routed TableInfo column
schemas (implement or call a helper like
areColumnSchemasCompatible(originTableInfo, routedTableInfo)); if schemas are
compatible (e.g., only partition/metadata changed) proceed processing the local
row using the routed TableInfo and log a warning; if incompatible, drop the
event and surface a sync-needed error (or keep the existing panic only for
confirmed incompatible schema changes) so consumers can trigger a re-sync. Also
align behavior with the remote-rows path (the guard around lines 315–323) to
ensure consistent handling of version mismatches.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6c0eaaa4-def4-4a8a-8451-be89b07e3c0b

📥 Commits

Reviewing files that changed from the base of the PR and between b90a0aa and a1b3e84.

📒 Files selected for processing (10)
  • downstreamadapter/eventcollector/dispatcher_stat.go
  • pkg/common/event/active_active_test.go
  • pkg/common/event/ddl_event.go
  • pkg/common/event/ddl_event_test.go
  • pkg/common/event/dml_event.go
  • pkg/common/event/handshake_event.go
  • pkg/common/event/util.go
  • pkg/common/table_info.go
  • pkg/common/table_info_test.go
  • pkg/sink/mysql/sql_builder_test.go
💤 Files with no reviewable changes (5)
  • pkg/common/event/ddl_event.go
  • pkg/common/event/active_active_test.go
  • pkg/common/event/util.go
  • pkg/common/event/handshake_event.go
  • pkg/common/event/ddl_event_test.go
✅ Files skipped from review due to trivial changes (1)
  • pkg/common/table_info_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/sink/mysql/sql_builder_test.go
  • downstreamadapter/eventcollector/dispatcher_stat.go

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/retest

@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. labels May 9, 2026
@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand 3AceShowHand requested a review from lidezhu May 9, 2026 07:14
@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/retest

Comment thread downstreamadapter/eventcollector/dispatcher_stat.go
@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test pull-cdc-kafka-integration-light

@3AceShowHand 3AceShowHand requested a review from wk989898 May 9, 2026 08:33
@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test pull-cdc-kafka-integration-light

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/retest

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test pull-cdc-kafka-integration-light

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/retest

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/test all

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/retest

2 similar comments
@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/retest

@3AceShowHand
Copy link
Copy Markdown
Collaborator Author

/retest

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

Labels

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.

integrate the table route functionality into the mysql sink

3 participants