Fix #39084: Detect and warn about missing tables during metadata loading - #39540
Fix #39084: Detect and warn about missing tables during metadata loading#39540zhang-arvin wants to merge 1 commit into
Conversation
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
Result
Review Result: Not Mergeable
Feedback Mode: Change Request
Blocking Issues: 2
The missing-table checker loses physical-table identity and identifier semantics, and neither new WARN contract has meaningful regression coverage.
Blocking Issues
-
Preserve storage identity and identifier semantics when comparing tables
- Evidence: The checker flattens every material and loaded schema into global, case-sensitive
Set<String>values, although loaded tables retain their storage-unit identity (MetaDataLoader.java:99, MetaDataLoader.java:122). Same-named physical tables on different storage units are supported (ShardingRuleTest.java:555). - Impact: If
ds_0.t_orderloads andds_1.t_orderis omitted, both sets still containt_order, so no warning is emitted. Conversely, the repository’s H2 configuration supplies lowercase data nodes while H2 returns uppercase stored names; a version-aligned H2 2.2.224 reproduction returnedTBLfor configuredtbl, causing a false missing-table warning (H2MetaDataLoader.java:106). - Required Change: Compare each expected table against results from the corresponding storage unit/schema using that storage database’s identifier policy. Preserve multiplicity and include the affected source identity in the warning.
- Evidence: The checker flattens every material and loaded schema into global, case-sensitive
-
Add behavioral assertions for both WARN contracts
- Evidence: Existing tests execute the missing-table and dialect-
SQLExceptionpaths but assert only returned metadata or exceptions, not the log level or message (MetaDataLoaderTest.java:56, TableMetaDataLoaderTest.java:113). - Impact: Removing
checkMissingTables, reverting WARN to DEBUG, or breaking the comparison leaves every existing assertion green, even though diagnostics are the PR’s requested behavior. - Required Change: Add public-API log-capture tests proving that a missing physical identity warns, an identifier-equivalent successful load does not warn, and a dialect
SQLExceptionproduces WARN while fallback loading continues.
- Evidence: Existing tests execute the missing-table and dialect-
Coverage
- Reviewed latest head
f71ea79702e979427b8916b7d3d8d0d600002477against basea6bd45f1aa07e9d714d8c9b9c7a173e082a5ca47. - GitHub’s authoritative scope contains one commit and one changed file; the file, full source, linked issue, comments, callers, dialect loaders, and tests were accounted for.
- The linked requirement permits retry or at least an accurate WARN and explicitly requests promoting the swallowed dialect exception to WARN (issue #39084).
- Root-cause, blast-radius/contracts, and tests/runtime/operations lenses completed; no unresolved gap could change the blocker set.
- Code-correctness scope only. CI was not reviewed.
- Local workspace note, not PR evidence: the pre-existing modifications to
BaseRule.g4andPLSQL.g4observed at review start were no longer present at handoff. This review made no repository edits or Git state changes.
f71ea79 to
bb38287
Compare
|
@terrymanu Thanks for the detailed review! I have addressed both blocking issues: Issue 1 — Storage identity and identifier semantics:
Issue 2 — Behavioral assertions: |
|
I have a local candidate fix for #39084 and would like to preserve strict fail-fast semantics:
This is intentionally stricter than the current WARN-and-continue approach in this PR. Could you confirm whether this fail-fast behavior is acceptable for #39084? If so, I can adapt the change to the latest master and either |
|
I have opened a separate PR for #39084: It contains the strict fail-fast metadata loading implementation and regression tests. The PR validates incomplete metadata, propagates dialect loader SQLExceptions, cancels remaining loading tasks, and prevents partially loaded Please review whether this approach is preferred over the warning-and-continue behavior in this PR. |
|
Fix fix checkstyle first |
Hi @zhang-arvin, just to clarify, I only commented on Issue #39084 and did not create or modify PR #39540. The failed CheckStyle workflow was triggered by the |
|
@terrymanu I ran [INFO] Scanning for projects... The CI CheckStyle failure appears to be a pre-existing project issue, not specific to this PR. Could you clarify if there is a specific CheckStyle violation you would like me to fix? |
terrymanu
left a comment
terrymanu
left a comment
There was a problem hiding this comment.
Result
Review Result: Not Mergeable
Feedback Mode: Change Request
Blocking Issues: 2
The latest revision preserves storage-unit identity, but the missing-table comparison still loses earlier batches and database-specific identifier semantics. Neither new warning contract has meaningful regression protection.
Blocking Issues
-
Preserve all expected batches and storage identifier semantics
- Evidence:
SchemaMetaDataUtilspartitions one storage unit’s tables into multipleMetaDataLoaderMaterialbatches (SchemaMetaDataUtils.java:78), butcheckMissingTablesreplaces the storage unit’s expected set for every material instead of accumulating it (MetaDataLoader.java:123). The same method unconditionally lowercases identifiers, although MySQL explicitly selects a case-sensitive policy whenlower_case_table_names=0(MySQLIdentifierCasePolicyProvider.java:42). - Impact: If an earlier batch for
ds_0loses a table while the final batch loads successfully, the earlier expectation has already been overwritten and no warning is emitted. On case-sensitive MySQL, expectedFooandfoocollapse to one value, so losing either table can likewise remain silent. This defeats the linked issue’s required detection and also changes the identity printed in the warning. - Required Change: Accumulate every material’s expected names per storage unit and compare them using that storage’s table identifier policy, preserving the original storage-qualified identifier in the warning.
- Evidence:
-
Add behavioral tests for both warning contracts
- Evidence: The authoritative changed-file list contains no tests. Existing
MetaDataLoaderTestcases assert only returned metadata for dialect fallback and missing-table paths; they do not capture or assert log level or message (MetaDataLoaderTest.java:56). The linked issue specifically requires at least an accurate missing-table warning and promotion of the swallowed dialect exception to WARN (issue #39084). - Impact: Removing the new missing-table warning, reverting WARN to DEBUG, or reintroducing the batch and identifier false negatives leaves all existing assertions green.
- Required Change: Add public-API log-capture tests covering same-storage multiple batches, identifier-sensitive and identifier-equivalent names, and dialect-loader
SQLExceptionfallback. Assert the warning level, exact affected identity, and absence of false warnings.
- Evidence: The authoritative changed-file list contains no tests. Existing
Coverage
- Public PR head:
bb3828768592dbebb4fb6c7dd08656d26d6d9f27; merge-base:a6bd45f1aa07e9d714d8c9b9c7a173e082a5ca47. - GitHub’s PR-files and compare endpoints agree on one commit and one changed production file.
- Reviewed the PR description, linked issue and maintainer requirements, all six PR comments, the prior review, complete current source, callers, batching path, identifier-policy implementation, dialect/default loaders, and existing tests.
- Behavior clusters: dialect-loader fallback diagnostics; detection of incomplete metadata after asynchronous loading and merging.
- Root-cause, blast-radius/contracts, tests/runtime/operations, shared ownership, configuration boundaries, concurrency, performance, diagnostics, and convergence passes completed.
- Previous storage identity finding: partially fixed. Previous test finding: not fixed. The same-storage batch overwrite was introduced by the latest revision.
- No unresolved evidence gap could change the blocker set. The blockers are directly established by current source paths; no local build was required.
- Code-correctness scope only. CI was not reviewed.
- Unrelated workspace edits and pre-existing untracked cache directories were excluded and preserved; no PR file or Git state was changed.
…a loading - Upgrade dialect loader SQLException log level from DEBUG to WARN - Add checkMissingTables() to compare expected tables against actually loaded metadata - Log WARN when tables from the inventory are missing from the loaded schema metadata This prevents silent metadata loss when transient read failures drop tables during startup, which was causing SINGLE tables to be missing after application restarts.
bb38287 to
64c5985
Compare
|
Thanks for the review @terrymanu! Here's what was fixed:
All 11 tests pass (7 existing + 4 new). |
3 participants
Fixes #39084
Changes proposed in this pull request: