You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@shardingsphere.apache.org by "menghaoranss (via GitHub)" <gi...@apache.org> on 2026/05/13 16:30:45 UTC

[PR] Refactor metadata reload flow and fix related tests [shardingsphere]

menghaoranss opened a new pull request, #38690:
URL: https://github.com/apache/shardingsphere/pull/38690

Changes proposed in this pull request:

This PR refines metadata reload and persistence behavior after storage unit/rule changes, and updates related tests in `mode/core`, `mode/type/cluster/core`, and `mode/type/standalone/core`.

## Motivation
The previous reload flow could persist schema/table metadata too early in some paths.
If step-1 operations failed (for example during storage-unit unload), in-memory schema tables and registry-persisted schema tables could become inconsistent across nodes.

This PR enforces a safer sequencing:
1. apply storage/rule changes,
2. rebuild schema index from updated runtime metadata,
3. compare old/new schema tables and persist deltas in the second stage.

## Main Functional Changes

### 1) Core metadata reload flow (`mode/core`)
- Refined `MetaDataContextsFactory` related rebuild entry points.
- Removed/cleaned obsolete checked-exception behavior on changed-database creation path.
- Kept `ContextManager` reload call-site semantics that still require the non-registry direct reload behavior (the existing `false` path expectation).

### 2) Cluster persistence path (`mode/type/cluster/core`)
- In `ClusterMetaDataManagerPersistService`, adjusted persistence behavior to rely on rebuilt schema index in the reload stage instead of unsafe direct full-database persistence ordering.
- Ensured schema/table persistence is aligned with post-update rebuild results.

### 3) Standalone persistence path (`mode/type/standalone/core`)
- Aligned standalone behavior with the same staged schema persistence principle.
- `StandaloneMetaDataManagerPersistService` uses rebuilt schema index in reload-related persistence paths consistently.

## Test Fixes

### `mode/core` tests
- Updated exception expectations after checked-exception path cleanup.
- Fixed tests around metadata/rule refresh:
- `MetaDataContextsFactoryTest`
- `DatabaseRuleConfigurationManagerTest`
- related `ContextManager` / `StorageUnitManager` tests impacted by reload semantics.

### `mode/type/standalone/core` tests
- Fixed `StandaloneMetaDataManagerPersistServiceTest` for schema-index rebuild prerequisites:
- added valid protocol type in mock database where needed,
- added rule attributes stubs (`RuleAttributes`) where rebuild path requires them,
- replaced fragile deep-stub metadata path with concrete `ShardingSphereMetaData` + `ConfigurationProperties` in affected cases.

### `mode/type/cluster/core` tests
- Fixed `ClusterMetaDataManagerPersistServiceTest.assertAlterRuleConfiguration`:
- provided protocol type (`MySQL`) for schema rebuild,
- provided non-null rule attributes,
- provided concrete metadata props context to avoid NPE during schema rebuild material creation.

## Behavior/Consistency Impact
- Reduces risk of divergence between:
- in-memory schema/table metadata of running nodes, and
- registry-persisted schema/table metadata loaded by newly started nodes.
- Keeps rule/storage update + metadata persistence ordering deterministic.

## Verification
Targeted tests executed for impacted areas, including:
- `DatabaseRuleConfigurationManagerTest`
- `MetaDataContextsFactoryTest`
- `StandaloneMetaDataManagerPersistServiceTest`
- `StandaloneContextManagerBuilderTest`
- `StandalonePersistServiceFacadeBuilderTest`
- `StandaloneProcessPersistServiceTest`
- `ClusterMetaDataManagerPersistServiceTest#assertAlterRuleConfiguration`

All targeted runs passed in the validated environment after fixes.

---

Before committing this PR, I'm sure that I have checked the following options:
- [ ] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [ ] I have self-reviewed the commit code.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [ ] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
- [ ] I have made corresponding changes to the documentation.
- [ ] I have added corresponding unit tests for my changes.
- [ ] I have updated the Release Notes of the current development version. For more details, see [Update Release Note](https://shardingsphere.apache.org/community/en/involved/contribute/contributor/)

--
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Refactor metadata reload flow and fix related tests [shardingsphere]

Posted by "menghaoranss (via GitHub)" <gi...@apache.org>.

menghaoranss merged PR #38690:
URL: https://github.com/apache/shardingsphere/pull/38690


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Refactor metadata reload flow and fix related tests [shardingsphere]

Posted by "menghaoranss (via GitHub)" <gi...@apache.org>.

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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org