Fix wrong logic table metadata when config same actual table name in different storage unit#39157
Fix wrong logic table metadata when config same actual table name in different storage unit#39157ClaireLytt wants to merge 8 commits into
Conversation
…different storage unit
terrymanu
left a comment
There was a problem hiding this comment.
Summary
Review Result: Not Mergeable
Feedback Mode: Change Request
Reason: The storage-unit-qualified lookup is the right direction, but the latest head introduces one shared aggregation regression and leaves two production branches using ambiguous table identity.
Issues
P1 — Storage-unit provenance becomes a metadata mismatch
Problem: TableMetaData includes the new storageUnitName in Lombok-generated toString(). The loader assigns a different value to each storage unit, while SchemaTableMetaDataAggregator uses toString() as its structural-equality check.
Impact: With documented property check-table-metadata-enabled=true, identical actual-table schemas from ds_0 and ds_1 are rejected as different. A current-head reproduction throws RuleAndStorageMetaDataMismatchedException. This also affects other users of the shared aggregator, including broadcast metadata.
Required Change: Please keep storage-unit provenance out of structural metadata comparison, or replace the toString() comparison with an explicit structural comparison. Add coverage for identical schemas loaded from different storage units with metadata checking enabled.
P1 — Qualified identity is used only for the table name
Problem: TableMetaDataReviseEngine passes storageUnitName to TableNameReviser, but column, index, and constraint revision still receive only the ambiguous actual table name. ShardingMetaDataReviseEntry therefore selects those revisers through the original unqualified lookup.
Impact: For t_order0 -> ds_0.t_order and t_order1 -> ds_1.t_order with different generated-key columns, current head correctly names the second table t_order1 but marks t_order0’s generated-key column in its metadata. The resulting logical table is internally inconsistent.
Required Change: Please propagate the storage-unit-qualified sharding-table identity through all sharding metadata revision branches, preferably resolving the owning ShardingTable once and reusing it. Add a regression test with different per-table metadata for the two storage units.
P1 — A qualified miss falls back to the ambiguous lookup
Problem: ShardingTableNameReviser calls the old unqualified lookup when a non-null storage-unit context has no matching sharding table.
Impact: If ds_0.t_order belongs to sharding table t_order0 while ds_1.t_order belongs to another rule, the qualified lookup correctly returns empty, but the fallback still renames the ds_1 table to t_order0. Because the metadata engine applies the sharding reviser to every loaded table, this preserves the original table-loss behavior in mixed-rule configurations.
Required Change: When storage-unit context is available and the qualified lookup misses, please preserve the original table name. Retain the unqualified fallback only when origin context is genuinely unavailable, if compatibility requires it. Add mixed-rule coverage for this branch.
Review Details
- Review Focus: Code Correctness Review. CI not reviewed by request.
- Reviewed Scope: All nine files from GitHub
/pulls/39157/files:RELEASE-NOTES.md; the connector-core loader and metadata model; the infra-common reviser interface, engine, and test; and the sharding-core reviser and rule classes. Latest head:4d49eeea3e3b2b3e2d04dd209f30d8f2ce0c2524; base ref:master; GitHub-reported base SHA:6ecd9c15b0ea089c6036fa4d1776098a19b03cb1; local merge-base:cb50781bc2a08ba20d7b9a66ec5e017fc2bf17c5. The local triple-dot file list matched GitHub exactly. - Not Reviewed Scope: GitHub Actions/check-runs and full Proxy/database E2E smoke were not reviewed. No dependency, packaging, license, or distribution files changed.
- Verification: The focused latest-head Maven run covering connector-core, infra-common, and sharding-core completed successfully with 89 tests and exit code 0. Compiled-bytecode inspection confirmed that
storageUnitNameparticipates inTableMetaData.toString(). Three bounded current-head reproductions confirmed the aggregation exception, cross-rule generated-column selection, and qualified-miss fallback described above. Public PR, issue, file, commit, comment, and review data were available; there is no relevant GitHub evidence gap. - Release Note / User Docs: The release-note entry is present. Additional user documentation is not required because the intended fix introduces no configuration or migration change.
|
@terrymanu Thank you for your review suggestion. Here are new changes. P1 — Qualified identity is used only for the table name P1 — A qualified miss falls back to the ambiguous lookup |

for #24982
Changes proposed in this pull request:
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.