config: table-router pr1 add changefeed level configuration#4654
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
📝 WalkthroughWalkthroughAdds per-rule table routing fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Model as "API model"
participant Converter as "API↔Internal Converter"
participant SinkConfig as "SinkConfig"
participant Validator as "validateRoutingExpression"
participant Errors
Client->>API_Model: submit DispatchRule with TargetSchema/TargetTable
API_Model->>Converter: ToInternalReplicaConfig()
Converter->>SinkConfig: populate internal DispatchRule (includes targets)
SinkConfig->>Validator: validateTableRoute() → validateRoutingExpression()
alt expression invalid
Validator->>Errors: return ErrInvalidTableRoutingRule
Errors-->>Client: validation error
else expression valid
SinkConfig-->>Client: accept config
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
ti-chi-bot
Bot
added
release-note-none
![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 capabilities by adding TargetSchema and TargetTable fields to the DispatchRule configuration. These fields allow users to specify downstream schema and table names using literal text or placeholders like {schema} and {table}. The changes include updates to the API models, configuration structures, and validation logic to support these new routing rules across different sink types. Feedback suggests optimizing the validation path for MySQL-compatible sinks to avoid unnecessary MQ-specific checks and refining the routing expression regex to ensure only valid identifier characters are used.
![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 (1)
pkg/config/sink_test.go (1)
298-355: Add onevalidateAndAdjust()case for MySQL-compatible sinks.These cases only call
validateTableRoute()directly, so they will not catch a future reorder that moves Line 830 back below the MySQL/TiDB early return. Please add onevalidateAndAdjust()test with atidb://ormysql://URI and an invalidtarget-schema/target-table.🧪 Example coverage
+func TestValidateAndAdjustRejectsInvalidTableRouteForTiDB(t *testing.T) { + t.Parallel() + + sinkURI, err := url.Parse("tidb://127.0.0.1:4000") + require.NoError(t, err) + + cfg := &SinkConfig{ + DispatchRules: []*DispatchRule{ + { + Matcher: []string{"db1.*"}, + TargetSchema: "{bad}", + }, + }, + } + + err = cfg.validateAndAdjust(sinkURI) + require.ErrorContains(t, err, "target-schema") +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/sink_test.go` around lines 298 - 355, Add a new table-route test that invokes SinkConfig.validateAndAdjust() (not just validateTableRoute()) for a MySQL-compatible sink (use a URI like "mysql://..." or "tidb://...") and include a DispatchRule with an invalid TargetSchema or TargetTable (e.g., "{bad}" or invalid placeholder) so the test expects an error containing "target-schema" or "target-table"; this ensures the early-return path for MySQL/TiDB sinks is exercised and will catch regressions where the validation order changes. Reference SinkConfig, validateAndAdjust, DispatchRule, and TargetSchema/TargetTable when locating where to add the case.
🤖 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/config/changefeed.go`:
- Around line 500-508: The loop in RmUnusedFields() that iterates over
info.Config.Sink.DispatchRules blindly dereferences each element and can panic
if a persisted null entry was unmarshaled; modify the loop that touches
info.Config.Sink.DispatchRules so it first checks for nil (e.g., if rule == nil
{ continue }) before clearing fields like DispatcherRule, PartitionRule,
IndexName, Columns, and TopicRule, ensuring nil entries are skipped rather than
dereferenced.
---
Nitpick comments:
In `@pkg/config/sink_test.go`:
- Around line 298-355: Add a new table-route test that invokes
SinkConfig.validateAndAdjust() (not just validateTableRoute()) for a
MySQL-compatible sink (use a URI like "mysql://..." or "tidb://...") and include
a DispatchRule with an invalid TargetSchema or TargetTable (e.g., "{bad}" or
invalid placeholder) so the test expects an error containing "target-schema" or
"target-table"; this ensures the early-return path for MySQL/TiDB sinks is
exercised and will catch regressions where the validation order changes.
Reference SinkConfig, validateAndAdjust, DispatchRule, and
TargetSchema/TargetTable when locating where to add the case.
🪄 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: 98b286d2-c3b9-422d-9076-fb31ae17a91f
📒 Files selected for processing (6)
api/v2/model.gopkg/config/changefeed.gopkg/config/sink.gopkg/config/sink_test.gopkg/errors/error.gotests/integration_tests/api_v2/model.go
|
/test all |
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/config/sink.go`:
- Around line 886-899: The validateTableRoute method should guard against nil
entries in s.DispatchRules to avoid panics: in SinkConfig.validateTableRoute
iterate over s.DispatchRules and if a rule is nil return a clear validation
error (e.g., "dispatch rule is null") instead of dereferencing it; then continue
validating rule.TargetSchema and rule.TargetTable as before—this same nil-check
will also protect subsequent dispatcher/partition normalization loops that
assume non-nil DispatchRule objects.
🪄 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: 65364691-1047-48b7-8379-bccfb59a46f7
📒 Files selected for processing (2)
pkg/config/changefeed.gopkg/config/sink.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/changefeed.go
ti-chi-bot
Bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flowbehappy, wk989898 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
/test all |
3AceShowHand
changed the title
3AceShowHand
changed the title
|
/test pull-cdc-mysql-integration-light-next-gen |
|
/retest |
|
/override pull-cdc-mysql-integration-light-next-gen |
|
@tenfyzhong: Overrode contexts on behalf of tenfyzhong: pull-cdc-mysql-integration-light-next-gen DetailsIn response to this:
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. |
|
@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. |
4 participants
What problem does this PR solve?
Issue Number: close #4655
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