terrymanu commented on PR #40050: URL: https://github.com/apache/shardingsphere/pull/40050#issuecomment-5979779732
### Result **Review Result: Mergeable** The local candidate satisfies the approved scope and compatibility policy. No blocking issues were found. This conclusion covers code readiness only; remote CI was not reviewed. ### Evidence - The complete call chain was reviewed: JDBC/Proxy → DistSQLUpdateExecuteEngine → LockClusterExecutor → ClusterLockStrategy. State checks, exclusive operations, and state updates all use the caller’s ContextManager. WRITE persists READ_ONLY; READ_WRITE persists UNAVAILABLE. - Both built-in strategies and their sole service registration were moved into the shared handler. The implementations have no fields and retain no caller context. TypedSPILoader, strategy names, and @SingletonSPI remain unchanged. Tests verify that the shared singleton handles two independent contexts correctly. - The Cluster mode restriction, missing and unknown algorithm errors, default and explicit timeouts, and callback state recheck are preserved. Existing assertions were not weakened. New consumer tests exercise the actual engine and strategy loading path. - All 79 focused and regression tests and 7 existing JDBC DistSQL integration tests passed. After the final test corrections, the 6 JDBC and 8 Proxy consumer cases were rerun successfully. - A clean build from current source and checks of both distributions passed. The JDBC/Proxy archives contained 177/350 JARs respectively. Each archive has one strategy service descriptor, both implementations come exclusively from the shared handler, and no old implementations or registrations remain. Relevant embedded JAR hashes match the newly built artifacts. - Spotless and Checkstyle passed after the last edit, covering all 10 current Java files. Checkstyle XML and cache records matching current source timestamps were independently checked. Apache RAT and test rule scans also passed. - Independent defensive-code audit: none. Production changes are limited to the method parameter, caller context propagation, implementation package and registration migration, and SPI contract documentation. No validation, copying, wrapping, fallback, exception handling, retry, synchronization, or lifecycle protection was added, extended, or strengthened. Existing state and algorithm checks remain unchanged. - No runtime operations, persistence writes, allocations, or synchronization mechanisms were added. There is no credible cost increase requiring a performance benchmark. ### Coverage Standalone local candidate. The authoritative baseline is the original file content saved for this task. The analysis HEAD was f35a8b13879d1cdfeae42d1ce5d347f9fd665f2b; the implementation HEAD was 120dbe5f79c43576e1cf03c407779bc79bef1fa0. The original 11 files were independently confirmed to match the implementation HEAD byte for byte. Six relocations were completed, and all 17 physical paths and their task changes were reviewed. The review covered the SPI contract, executor context ownership, built-in strategies and service loading, six test classes, JDBC/Proxy consumers, and distribution paths. Root cause, compatibility and impact, and tests and runtime delivery were reviewed, followed by a final convergence check. No new independent issues or unresolved evidence gaps remain. The method signature and implementation package changes are explicitly approved compatibility changes. Old custom strategies must update their signatures and be recompiled; this does not establish binary compatibility with old plugins. JDBC production code and ordinary SQL admission rules for cluster state were not modified. No modules, dependencies, compatibility bridges, or parallel SPIs were added. Pre-existing unrelated changes were preserved. -- 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]
