event: table-router pr2 - support table route functionality on the event model#4658
event: table-router pr2 - support table route functionality on the event model#46583AceShowHand wants to merge 16 commits intomasterfrom
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes add runtime-only routing support to DDL and DML events through new unexported fields and getter methods, enabling schema/table name mapping for sink output paths. New constructors and conversion methods preserve both origin and routed table identities across redo-log persistence and event-splitting operations. Changes
Sequence DiagramsequenceDiagram
participant Source as Source Event
participant TableInfo as TableInfo<br/>(Routing)
participant DDLEvent as Routed<br/>DDLEvent
participant GetEvents as GetEvents<br/>Splitting
participant Redo as Redo<br/>Serialization
participant Restored as Restored<br/>DMLEvent
Source->>TableInfo: CloneWithRouting<br/>(targetSchema, targetTable)
TableInfo->>DDLEvent: NewRoutedDDLEvent<br/>(source, tableInfo)
Note over DDLEvent: Sets targetSchemaName<br/>targetTableName from<br/>routing
DDLEvent->>GetEvents: GetEvents()<br/>(per-table split)
GetEvents->>GetEvents: Extract old names<br/>from RENAME query
Note over GetEvents: Sets target fields on<br/>each split event
GetEvents->>Redo: Each split event<br/>→ RedoDDLEvent
Redo->>Redo: Serialize with<br/>TargetSchema/Table
Redo->>Restored: Decode & restore<br/>CloneWithRouting
Restored->>Restored: Populate targetSchemaName<br/>from persisted TargetSchema
Note over Restored: Origin & target both<br/>preserved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces table routing support by adding TargetSchema and TargetTable fields to the DispatchRule configuration. The changes involve updating DDLEvent, DMLEvent, and TableInfo to handle routed names, implementing a CloneForRouting mechanism to prevent race conditions, and ensuring routing information is preserved in redo logs. Additionally, validation logic for routing expressions using placeholders like {schema} and {table} has been added. Feedback was provided regarding the use of log.Panic in the DDL name extraction logic, suggesting a refactor to return errors for improved robustness.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/common/event/dml_event.go (1)
285-286: Use neutral log messages without type/function prefixes.The panic messages in Line 285 and Line 309 include
DMLEvent:prefixes; prefer concise semantic messages with structured fields only.As per coding guidelines "Use structured logs via
github.com/pingcap/logwithzapfields in Go; log message strings should not include function names and should avoid hyphens (use spaces instead)".Also applies to: 309-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/event/dml_event.go` around lines 285 - 286, Replace the panic messages that include the "DMLEvent:" prefix with neutral, structured log calls: instead of log.Panic("DMLEvent: TableInfo is nil") and the similar messages around the other log.Panic at the TableInfo checks, call log.Panic with a short message like "table info is nil" and pass context as zap fields (e.g., zap.String("event", "dml"), zap.Any("tableInfo", TableInfo) or relevant IDs) using github.com/pingcap/log; remove function/type prefixes from the message and use spaces rather than hyphens.pkg/common/event/ddl_event_test.go (1)
592-614: Test assertion may not match the intended behavior.At line 593,
cloned.SchemaNameis set to"routed_schema", but at line 608, the test assertscloned.TargetSchemaNameequals"routed_schema". However,TargetSchemaNamewas never explicitly set in this test block.Looking at the code flow:
- Line 593:
cloned.SchemaName = "routed_schema"- Line 608:
require.Equal(t, "routed_schema", cloned.TargetSchemaName)Since
GetTargetSchemaName()falls back toSchemaNamewhenTargetSchemaNameis empty (based on the accessor logic), this assertion might pass but doesn't actually test theTargetSchemaNamefield being set correctly. Consider explicitly settingcloned.TargetSchemaName = "routed_schema"to make the test intention clearer.💡 Suggested clarification
// Now simulate what happens during routing: mutate the cloned event - cloned.SchemaName = "routed_schema" + cloned.TargetSchemaName = "routed_schema" cloned.Query = "CREATE TABLE routed_schema.test ..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/event/ddl_event_test.go` around lines 592 - 614, The test mutates cloned.SchemaName but then asserts cloned.TargetSchemaName equals "routed_schema", which relies on the accessor fallback rather than explicitly testing the TargetSchemaName field; update the test to set cloned.TargetSchemaName = "routed_schema" (in addition to or instead of cloned.SchemaName) before assertions so the intent to verify the TargetSchemaName field is explicit, and keep assertions that use cloned.TargetSchemaName (or use GetTargetSchemaName() if you intend to test fallback behavior).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/event/dml_event.go`:
- Around line 297-313: When assigning a new TableInfo for local routed batches
(inside the b.Rows != nil branch) you must reintroduce the schema-version guard
so we don't replace b.TableInfo with tableInfo if their UpdateTS differs; in the
b.Rows != nil block (handling local batches where Rows already exist) check if
b.TableInfo is non-nil and b.TableInfo.GetUpdateTS() != tableInfo.GetUpdateTS()
and panic/log an error like the remote-branch does instead of unconditionally
setting b.TableInfo and dml.TableInfo for each DMLEvent; update the code paths
that set b.TableInfo and each dml.TableInfo (referencing b.Rows, b.TableInfo,
tableInfo, and the DMLEvents slice) to perform this version check first.
---
Nitpick comments:
In `@pkg/common/event/ddl_event_test.go`:
- Around line 592-614: The test mutates cloned.SchemaName but then asserts
cloned.TargetSchemaName equals "routed_schema", which relies on the accessor
fallback rather than explicitly testing the TargetSchemaName field; update the
test to set cloned.TargetSchemaName = "routed_schema" (in addition to or instead
of cloned.SchemaName) before assertions so the intent to verify the
TargetSchemaName field is explicit, and keep assertions that use
cloned.TargetSchemaName (or use GetTargetSchemaName() if you intend to test
fallback behavior).
In `@pkg/common/event/dml_event.go`:
- Around line 285-286: Replace the panic messages that include the "DMLEvent:"
prefix with neutral, structured log calls: instead of log.Panic("DMLEvent:
TableInfo is nil") and the similar messages around the other log.Panic at the
TableInfo checks, call log.Panic with a short message like "table info is nil"
and pass context as zap fields (e.g., zap.String("event", "dml"),
zap.Any("tableInfo", TableInfo) or relevant IDs) using github.com/pingcap/log;
remove function/type prefixes from the message and use spaces rather than
hyphens.
🪄 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: 22df85cc-da4b-4e9b-816a-0c2e50dbf97d
📒 Files selected for processing (15)
api/v2/model.gopkg/common/event/ddl_event.gopkg/common/event/ddl_event_test.gopkg/common/event/dml_event.gopkg/common/event/redo.gopkg/common/event/redo_test.gopkg/common/table_info.gopkg/common/table_info_test.gopkg/common/table_name.gopkg/common/table_name_gen.gopkg/config/changefeed.gopkg/config/sink.gopkg/config/sink_test.gopkg/errors/error.gotests/integration_tests/api_v2/model.go
3AceShowHand
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/common/table_info.go (2)
184-189: Shadowed variable name in finalizer closure may cause confusion.The finalizer parameter
tishadows the outertivariable. While functionally correct (the parameter receivesclonedwhen finalized), this can confuse maintainers.✏️ Suggested rename for clarity
if ti.columnSchema != nil { GetSharedColumnSchemaStorage().incColumnSchemaCount(ti.columnSchema) - runtime.SetFinalizer(cloned, func(ti *TableInfo) { - GetSharedColumnSchemaStorage().tryReleaseColumnSchema(ti.columnSchema) + runtime.SetFinalizer(cloned, func(t *TableInfo) { + GetSharedColumnSchemaStorage().tryReleaseColumnSchema(t.columnSchema) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info.go` around lines 184 - 189, The finalizer closure in the TableInfo cloning code shadows the outer variable name `ti`, which is confusing; change the finalizer parameter name (e.g., use `t` or `tbl`) so it no longer shadows the outer `ti` and update the closure body to call GetSharedColumnSchemaStorage().tryReleaseColumnSchema(t.columnSchema) (or tbl.columnSchema) while keeping the call to GetSharedColumnSchemaStorage().incColumnSchemaCount(ti.columnSchema) and the runtime.SetFinalizer invocation with `cloned` unchanged.
128-134: Consider logging when returning early due to nil columnSchema.The silent early return makes debugging difficult. When
columnSchemais nil, this returns without settingisInitialized, and any subsequent call toGetPreInsertSQL(),GetPreReplaceSQL(), orGetPreUpdateSQL()will panic. Adding a debug log would help identify when this path is taken unexpectedly in production.💡 Suggested logging addition
// columnSchema may be nil for minimal TableInfo instances (e.g., in tests). // In production, columnSchema is always set via WrapTableInfo or similar. // Early return here without marking as initialized, so if columnSchema is // set later, InitPrivateFields can be called again to properly initialize. if ti.columnSchema == nil { + log.Debug("skipping preSQLs init due to nil columnSchema", + zap.String("schema", ti.TableName.Schema), + zap.String("table", ti.TableName.Table)) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/table_info.go` around lines 128 - 134, The early-return in InitPrivateFields when ti.columnSchema == nil should log a debug message before returning to aid debugging; update InitPrivateFields on type TableInfo (the method containing the columnSchema nil check) to call the package logger or a provided logger with a clear message including identifying context (e.g., table name or ti.ID if available) that columnSchema is nil and isInitialized remains false, then return; ensure the log level is debug/trace so tests/verging code aren’t noisy and do not change control flow or initialization semantics used by GetPreInsertSQL/GetPreReplaceSQL/GetPreUpdateSQL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/common/table_info.go`:
- Around line 184-189: The finalizer closure in the TableInfo cloning code
shadows the outer variable name `ti`, which is confusing; change the finalizer
parameter name (e.g., use `t` or `tbl`) so it no longer shadows the outer `ti`
and update the closure body to call
GetSharedColumnSchemaStorage().tryReleaseColumnSchema(t.columnSchema) (or
tbl.columnSchema) while keeping the call to
GetSharedColumnSchemaStorage().incColumnSchemaCount(ti.columnSchema) and the
runtime.SetFinalizer invocation with `cloned` unchanged.
- Around line 128-134: The early-return in InitPrivateFields when
ti.columnSchema == nil should log a debug message before returning to aid
debugging; update InitPrivateFields on type TableInfo (the method containing the
columnSchema nil check) to call the package logger or a provided logger with a
clear message including identifying context (e.g., table name or ti.ID if
available) that columnSchema is nil and isInitialized remains false, then
return; ensure the log level is debug/trace so tests/verging code aren’t noisy
and do not change control flow or initialization semantics used by
GetPreInsertSQL/GetPreReplaceSQL/GetPreUpdateSQL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b933a0b-2235-48ba-b989-3b3434032d2d
📒 Files selected for processing (5)
pkg/common/event/ddl_event.gopkg/common/event/ddl_event_test.gopkg/common/event/redo_test.gopkg/common/table_info.gopkg/common/table_info_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/common/event/redo_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/common/table_info_test.go
- pkg/common/event/ddl_event_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/table_info.go`:
- Around line 395-406: The change made GetSchemaName/GetTableName to return
origin names breaks routing logic; revert GetSchemaName and GetTableName in
TableInfo to remain routing-aware (i.e., return TargetSchema/TargetTable when
set, falling back to origin if not) so existing callers in basic_dispatcher.go,
dml_writers.go and pulsar sink keep using routed names, and add new explicit
methods GetOriginSchema and GetOriginTable (and a GetOriginTableCIStr if needed)
that call TableName.GetOriginSchema()/GetOriginTable() for code that needs
upstream names; update GetTableNameCIStr to wrap the routing-aware GetTableName
(or add a separate GetOriginTableCIStr) to avoid changing existing behavior.
🪄 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: 120b2817-9977-406f-9350-5c096dddc689
📒 Files selected for processing (5)
pkg/common/event/ddl_event.gopkg/common/event/ddl_event_test.gopkg/common/event/redo_test.gopkg/common/table_info.gopkg/common/table_info_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/common/event/redo_test.go
- pkg/common/event/ddl_event_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/common/table_info_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/event/ddl_event.go`:
- Around line 51-56: DDLEvent's runtime-only fields
targetSchemaName/targetTableName/targetExtraSchemaName/targetExtraTableName are
excluded from JSON (json:"-") so they are lost through
DDLEvent.Marshal()/Unmarshal(), causing GetTarget*() to fall back to upstream
names; update DDLEvent.Marshal and Unmarshal to include and restore these fields
(or remove the json:"-" tags) so routed names are persisted, or alternatively
rehydrate them during Unmarshal by deriving values from the embedded TableInfo
or the routed query logic used elsewhere; reference the DDLEvent type, its
Marshal()/Unmarshal() methods, GetTarget*() accessors and TableInfo when
implementing the fix.
- Around line 44-56: The canonical DDL name lookup in
pkg/sink/codec/common/ddl.go still reads origin fields ddl.SchemaName and
ddl.TableName, so sinks will continue to emit source identifiers; update the
lookup and any helper that derives output identifiers (the canonical DDL name
helper used around the ranges 157-175, 214-240, and 358-363) to use the routed
accessors on the DDL event instead of the origin fields—call the event's routed
getters (e.g. TargetSchemaName()/TargetTableName() and
TargetExtraSchemaName()/TargetExtraTableName() or whatever the exported accessor
names are) wherever ddl.SchemaName, ddl.TableName, ddl.ExtraSchemaName, or
ddl.ExtraTableName are currently used so sink output uses the routed names.
In `@pkg/common/event/redo.go`:
- Around line 54-57: The redo DDL payload currently only stores TableName and
reconstructs rename/origin info from TableInfo, which loses
ExtraSchemaName/ExtraTableName and routed targets (and breaks when
TableInfo==nil); update the redo struct (the container with fields DDL, Type,
TableName, TableSchemaStore) to include explicit origin/target name fields
(e.g., OriginSchema, OriginTable, TargetSchema, TargetTable and their routed
equivalents), ensure ToRedoLog populates these new fields from the incoming
DDLEvent (not just from d.TableInfo) even when TableInfo is nil, and change
ToDDLEvent to prefer the persisted origin/target and routed name fields when
rebuilding the DDLEvent instead of inferring them from TableInfo alone so
rename-side and nil-TableInfo routed DDLs round-trip correctly.
🪄 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: 405968f1-20c0-4ca9-bbe7-6c3933bdbc6e
📒 Files selected for processing (5)
pkg/common/event/ddl_event.gopkg/common/event/ddl_event_test.gopkg/common/event/redo.gopkg/common/event/redo_test.gopkg/common/table_info.go
✅ Files skipped from review due to trivial changes (1)
- pkg/common/event/redo_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/common/event/ddl_event_test.go
- pkg/common/table_info.go
|
/test all |
|
/test all |
|
/test all |
|
@3AceShowHand: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
1 participant
What problem does this PR solve?
Issue Number: close #4702
What is changed and how it works?
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit
New Features
Improvements