menghaoranss commented on PR #39234: URL: https://github.com/apache/shardingsphere/pull/39234#issuecomment-5068204403
```markdown ### 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 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]
