Skip to content

[DNM]: test#12656

Closed
joechenrh wants to merge 3 commits into
pingcap:masterfrom
joechenrh:fix/pr12654-dm-ci
Closed

[DNM]: test#12656
joechenrh wants to merge 3 commits into
pingcap:masterfrom
joechenrh:fix/pr12654-dm-ci

Conversation

@joechenrh
Copy link
Copy Markdown
Contributor

Test-fix PR based on #12654.

Purpose: validate whether the failing DM integration CI is related to the route-aware FK relation path not being wired into syncer.

Change: pass syncer's route resolver into schema tracker FK relation initialization, so the new route-aware builder can validate route alignment instead of always using identity route.

Issue Number: ref #12654

Tests:

  • git diff --check
  • Local go test was attempted but stopped because first-time dependency compilation did not finish quickly; leaving full validation to CI.

Release note: None

@ti-chi-bot
Copy link
Copy Markdown
Contributor

ti-chi-bot Bot commented May 28, 2026

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-triage-completed do-not-merge/needs-linked-issue do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 28, 2026
@ti-chi-bot
Copy link
Copy Markdown
Contributor

ti-chi-bot Bot commented May 28, 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 benjamin2037 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 area/dm Issues or PRs related to DM. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 28, 2026
@joechenrh joechenrh changed the title syncer(dm): pass route resolver for FK relations [DNM]: test May 28, 2026
@ti-chi-bot
Copy link
Copy Markdown
Contributor

ti-chi-bot Bot commented May 28, 2026

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456.

📖 For more info, you can check the "Contribute Code" section in the development guide.

@joechenrh
Copy link
Copy Markdown
Contributor Author

Closing this test PR because it was based on #12654. I will create a separate master-based CI baseline PR instead.

@joechenrh joechenrh closed this May 28, 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 introduces route-aware foreign key causality tracking by adding a TableRouteResolver parameter to the downstream foreign key initialization and relation-building processes. This allows the schema tracker to correctly resolve and align routed source tables with their downstream counterparts. Additionally, the caching mechanism was updated to use source table IDs, and comprehensive unit tests were added. The reviewer feedback suggests simplifying a table name string conversion and removing the precheckForeignKeyCausality method, which has become dead code after its call was removed from the syncer.

Comment thread dm/pkg/schema/tracker.go

parentTableID := utils.GenTableID(targetParentTable)
parentTableName := (&cdcmodel.TableName{Schema: targetParentSchema, Table: fk.RefTable.O}).String()
parentTableName := (&cdcmodel.TableName{Schema: sourceParentSchema, Table: sourceFK.RefTable.O}).String()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The parentTableName can be simplified by calling sourceParentTable.String() directly, which avoids allocating a new cdcmodel.TableName struct and achieves the exact same string representation.

Suggested change
parentTableName := (&cdcmodel.TableName{Schema: sourceParentSchema, Table: sourceFK.RefTable.O}).String()
parentTableName := sourceParentTable.String()

Comment thread dm/syncer/syncer.go
return nil, err
}
return s.schemaTracker.InitDownStreamForeignKeyRelations(tctx, sourceTable, targetTable, originTI)
return s.schemaTracker.InitDownStreamForeignKeyRelations(tctx, sourceTable, targetTable, originTI, s.route)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Since the call to s.precheckForeignKeyCausality has been removed, the precheckForeignKeyCausality method defined around line 3730 is now unused dead code. Please remove its definition to keep the codebase clean.

@joechenrh joechenrh deleted the fix/pr12654-dm-ci branch May 28, 2026 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/dm Issues or PRs related to DM. do-not-merge/needs-linked-issue do-not-merge/needs-triage-completed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants