terrymanu commented on PR #39771: URL: https://github.com/apache/shardingsphere/pull/39771#issuecomment-5588790068
### Result **Review Result: Mergeable** The SPI refactor preserves existing ZooKeeper behavior, adds correct resource cleanup, and introduces no confirmed functional, compatibility, packaging, or performance regression. ### Evidence - Reviewed [PR #39771](https://github.com/apache/shardingsphere/pull/39771) at head `232ecf6444bbb82f0962cd8916e3f721dca9cbe7`. - The [SPI contract](https://github.com/apache/shardingsphere/blob/232ecf6444bbb82f0962cd8916e3f721dca9cbe7/kernel/schedule/core/src/main/java/org/apache/shardingsphere/schedule/spi/CoordinatorRegistryCenterProvider.java#L25-L42) defines caller ownership and initialization-failure cleanup; the [ZooKeeper provider](https://github.com/apache/shardingsphere/blob/232ecf6444bbb82f0962cd8916e3f721dca9cbe7/mode/type/cluster/repository/provider/zookeeper/src/main/java/org/apache/shardingsphere/mode/repository/cluster/zookeeper/ZookeeperCoordinatorRegistryCenterProvider.java#L31-L77) preserves namespace, timeout, retry, and digest configuration. - Pipeline initialization now gates unsupported providers and releases cached APIs and registry resources during destruction: [listener](https://github.com/apache/shardingsphere/blob/232ecf6444bbb82f0962cd8916e3f721dca9cbe7/kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/listener/PipelineContextManagerLifecycleListener.java#L52-L115), [factory](https://github.com/apache/shardingsphere/blob/232ecf6444bbb82f0962cd8916e3f721dca9cbe7/kernel/data-pipeline/core/src/main/java/org/apache/shardingsphere/data/pipeline/core/job/api/PipelineAPIFactory.java#L126-L177). - Statistics-worker initialization remains retryable and cleans partially initialized resources: [StatisticsCollectJobWorker](https://github.com/apache/shardingsphere/blob/232ecf6444bbb82f0962cd8916e3f721dca9cbe7/kernel/schedule/core/src/main/java/org/apache/shardingsphere/schedule/core/job/statistics/collect/StatisticsCollectJobWorker.java#L63-L140). - The encrypt cleanup retains case-insensitive table matching: [AlterEncryptRuleExecutor](https://github.com/apache/shardingsphere/blob/232ecf6444bbb82f0962cd8916e3f721dca9cbe7/features/encrypt/distsql/handler/src/main/java/org/apache/shardingsphere/encrypt/distsql/handler/update/AlterEncryptRuleExecutor.java#L105-L115). - Focused Maven verification passed: 35 tests, 0 failures, 0 errors. - Spotless and Checkstyle passed for all four changed owning modules. - Dependency resolution passed and selected ElasticJob ZooKeeper registry center version `3.0.4`. ### Coverage - Candidate: public PR; no local-only candidate delta. - Base: `master` at GitHub SHA `46769ed1a66de8a712a45fbe7230898ccf4f8ef1`; local triple-dot merge-base `7adedc38244089bdf4a41292f876991be10540c9`. - Scope: all 14 GitHub-listed files; the local triple-dot file list matched exactly. - Requirements: PR title and two commits; no PR body, linked issue, comments, or reviews were present. - Behavior clusters: encrypt altered-rule cleanup; pipeline registry-center lifecycle; statistics registry-center lifecycle; ZooKeeper SPI implementation and packaging. - Completed lenses: root cause and behavior; blast radius and contracts; tests, runtime, and operations; final full-scope convergence. - Unresolved evidence gaps: none. - This is a Code Correctness Review only; CI and GitHub Actions were not reviewed. -- 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]
