Add StorageUnitConfiguration to encapsulate DataSourcePoolProperties#39234
Open
menghaoranss wants to merge 1 commit into
Open
Add StorageUnitConfiguration to encapsulate DataSourcePoolProperties#39234menghaoranss wants to merge 1 commit into
menghaoranss wants to merge 1 commit into
Conversation
Contributor
Author
### Correction
- **Previous Finding:** Replacing the `DataSourceGeneratedDatabaseConfiguration` constructor could break external source or binary callers.
- **Current Status:** Withdrawn
- **Reason:** All repository references have been migrated to `StorageUnitConfiguration`. The class is under the internal `infra.config.database.impl` package, and the public evidence found only project-owned callers and the generated [ShardingSphere source xref](https://shardingsphere.apache.org/statistics/staging/shardingsphere-infra/shardingsphere-infra-common/xref/org/apache/shardingsphere/infra/config/database/impl/DataSourceGeneratedDatabaseConfiguration.html). There is no attributable external caller or documented supported-API contract. Therefore, the previous compatibility concern does not pass the blocker proof gate.
### Summary
**Review Result: Mergeable**
**Reason:** The latest code scope correctly introduces `StorageUnitConfiguration`, propagates it through YAML persistence, registry recovery, and Proxy database configuration, while preserving the existing metadata YAML structure and global data-source behavior. No code-level blocker remains. This result is code-scope only; CI was not reviewed by request.
### Evidence
- All repository constructor references use `Map<String, StorageUnitConfiguration>`.
- Existing `DatabaseConfiguration` implementations remain compatible through the default conversion method.
- Legacy metadata YAML is parsed through the existing `DataSourcePoolProperties` conversion path.
- New persistence still emits the existing data-source YAML structure.
- Proxy database data sources use the new configuration model; Proxy global data sources retain their previous path.
- Empty active-version and empty-version-content behavior remains covered.
- The latest-head focused test run passed 63 tests with no failures.
### Review Details
- **Review Focus:** Code Correctness Review. CI not reviewed by request.
- **Reviewed Scope:** All 20 changed files at head `a3a9c739584d364f4b79956ee98aa2507759a01e`; base and merge-base `43829a8d089cc4405b0177d1fee96f56293f6b94`. The local triple-dot file list matched GitHub `/pulls/39234/files`.
- **Not Reviewed Scope:** GitHub Actions, check runs, workflow logs, and external deployment environments.
- **Verification:** Spotless and Checkstyle passed. Focused tests across `infra/common`, `mode/core`, Cluster mode, Proxy backend, and CDC passed: 63 tests, 0 failures. The challenged compatibility premise was rechecked against all repository references and available public evidence.
- **Release Note / User Docs:** Not required. The persisted YAML format and user-facing Proxy configuration schema remain unchanged. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #ISSUSE_ID.
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.