menghaoranss commented on PR #38690:
URL: https://github.com/apache/shardingsphere/pull/38690#issuecomment-4445998639
1. Decision
- `Merge Verdict: Mergeable`
- `Reviewed Scope`:
- Metadata reload call chain in `mode/core`: `ContextManager`,
`MetaDataContextsFactory`, `StorageUnitManager`,
`DatabaseRuleConfigurationManager`
- Two-phase schema persistence in `mode/type/cluster/core`:
`ClusterMetaDataManagerPersistService`
- Parallel fix in `mode/type/standalone/core`:
`StandaloneMetaDataManagerPersistService`
- Related unit-test updates in core / cluster / standalone modules
- `Not Reviewed Scope`:
- I did not inspect every E2E job log line-by-line (only
conclusions/statuses).
- I did not run a local rebuild in this review turn.
- `Need Expert Review`: No (changes are in metadata reload/persistence
sequencing, not security/protocol-critical areas).
2. Root-cause coverage
- The PR goal and changed paths are aligned: move schema persistence to
phase-2 (after reload and schema-index rebuild) to avoid in-memory vs registry
inconsistency when phase-1 fails.
- Core fix coverage is complete:
- Removes `isLoadSchemasFromRegisterCenter` branching and replaces it with
explicit paths:
- `createChangedDatabaseByLoad(...)` (load schema from registry center)
- `createChangedDatabaseByRebuild(...)` (rebuild schema from data source
+ rules)
- Cluster/standalone persistence services now rebuild schema index first,
then persist via `persistReloadDatabase(...)` for storage-unit and rule
changes.
- This is a root-cause sequencing fix, not a fallback-only patch.
3. Risk scan
- Design consistency:
- Changes stay within metadata reload/persistence boundaries; no
cross-layer bypass introduced.
- Compatibility:
- No broad external API/SPI breakage observed; call sites are updated
coherently.
- Semantics improved from boolean-driven behavior to explicit method
intent.
- Regression surface:
- Storage-unit unregister and rule add/alter/remove flows are covered by
schema-index rebuild before persistence.
- Tests were adjusted to provide protocol/rule-attribute context, reducing
mock-shape false failures.
- Performance:
- Additional `GenericSchemaBuilder.build(...)` cost exists, but on
config-change paths (not query hot path), so acceptable.
4. Validation evidence
- PR facts: 1 commit, 12 files changed, `+175/-87`.
- GitHub checks on head `094239be7f638306ce8b35d250d0c1ca0574bf9c`:
- total: `44`
- success: `43`
- skipped: `1`
- failure: `0`
- Reviews API currently returns empty (no blocking review comments found).
5. Non-blocking follow-up
- Recommend adding one integration-style regression case with realistic rule
objects and protocol type to validate `rebuildDatabaseSchemaIndex(...)`
behavior parity between standalone and cluster flows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
