terrymanu commented on PR #38847: URL: https://github.com/apache/shardingsphere/pull/38847#issuecomment-4693917885
### Summary - **Merge Decision: Mergeable** - Reason: Latest PR head keeps the change inside MCP feature plugin planning contracts, and I found no root-cause, scope, regression, test, style, or release-note blocker. ### Issues - None. ### Evidence - The PR scope is cohesive: changes are limited to `mcp/features/broadcast`, `mcp/features/mask`, `mcp/features/readwrite-splitting`, `mcp/features/shadow`, and `mcp/features/sharding`; no shared MCP core/support API, dependency, parser, distribution, Proxy/JDBC high-frequency path, or runtime config path is changed. - The new broadcast single-table resource is registered through `mcp/features/broadcast/src/main/java/org/apache/shardingsphere/mcp/feature/broadcast/BroadcastMCPHandlerProvider.java:40`, filters by logical table, and returns parent navigation metadata in `mcp/features/broadcast/src/main/java/org/apache/shardingsphere/mcp/feature/broadcast/resource/handler/BroadcastTableRuleHandler.java:61`; `mcp/features/broadcast/src/test/java/org/apache/shardingsphere/mcp/feature/broadcast/resource/handler/BroadcastResourceHandlerTest.java:43` covers the resource response and navigation. - Readwrite-splitting status planning now rejects conflicting `target_status` / `operation_type` before DistSQL artifact generation in `mcp/features/readwrite-splitting/src/main/java/org/apache/shardingsphere/mcp/feature/readwritesplitting/tool/service/ReadwriteSplittingStatusWorkflowPlanningService.java:85`; the conflict path is covered by `mcp/features/readwrite-splitting/src/test/java/org/apache/shardingsphere/mcp/feature/readwritesplitting/tool/service/ReadwriteSplittingWorkflowPlanningServiceTest.java:127`. - Sharding component cleanup remains drop-only: `mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingWorkflowPlanningService.java:174` rejects non-drop cleanup before generating artifacts, and `mcp/features/sharding/src/test/java/org/apache/shardingsphere/mcp/feature/sharding/tool/service/ShardingWorkflowPlanningServiceTest.java:209` covers the negative case. - Descriptor validators now require the expanded planning output fields and workflow metadata for the changed feature plugins, with direct validator tests covering missing output fields and missing metadata. - Linked issue `https://github.com/apache/shardingsphere/issues/35294` is an umbrella MCP idea-collection issue, and this PR says `For #35294` rather than claiming to close it; I treated this as staged MCP planning-contract alignment and found no over-claim. - Adversarial scan did not find new shared-layer ownership leakage, target-specific semantics in shared modules, new feature flags, dependency/distribution impact, or `ConcurrentHashMap#computeIfAbsent` usage on Proxy/JDBC high-frequency SQL paths. ### Review Details - **Reviewed Scope:** PR `https://github.com/apache/shardingsphere/pull/38847`, head SHA `3b37c631ee9267b551e4d942669719f5ef8ab264`, local merge-base SHA `772ef4d2b1d55b216bd7ad84c6815aff750f62c7`. GitHub `/pulls/38847/files` matched the local triple-dot file list: 28 files under the five MCP feature plugin modules listed above. - **Not Reviewed Scope:** GitHub Actions/check-run status was intentionally not reviewed. I did not run full `./mvnw clean install -B -T1C -Pcheck`, MCP E2E, or a live Proxy/MCP server smoke. SQL parser dialect-family review is not applicable because no parser files changed. - **Verification:** `git fetch apache master pull/38847/head:refs/remotes/apache/pr/38847` exited 0. GitHub file-list cross-check script exited 0. `./mvnw -pl mcp/features/broadcast,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding -am -Dspotless.skip=true -Dtest=BroadcastWorkflowPlanningServiceTest,BroadcastRuleDistSQLPlanningServiceTest,BroadcastToolDescriptorValidatorTest,BroadcastResourceHandlerTest,MaskWorkflowPlanningServiceTest,MaskRuleDistSQLPlanningServiceTest,MaskToolDescriptorValidatorTest,ReadwriteSplittingWorkflowPlanningServiceTest,ReadwriteSplittingDistSQLPlanningServiceTest,ReadwriteSplittingToolDescriptorValidatorTest,ShadowWorkflowPlanningServiceTest,ShadowDistSQLPlanningServiceTest,ShadowToolDescriptorValidatorTest,ShardingWorkflowPlanningServiceTest,ShardingDistSQLPlanningServiceTest,ShardingToolDescriptorValidatorTest test -Dsurefire.failIfNoSpecifiedTests=false` exited 0. `./mvnw -pl mcp/bootstrap -am -Dspotless.skip =true -Dtest=MCPToolSpecificationFactoryTest,StreamableHttpMCPServletTest test -Dsurefire.failIfNoSpecifiedTests=false` exited 0. `./mvnw spotless:check -Pcheck -T1C` exited 0. `./mvnw checkstyle:check -Pcheck -T1C` exited 0. - **Release Note / User Docs:** Not required for this narrow contract-alignment PR because it does not change user configuration, DistSQL syntax, runtime behavior, migration requirements, dependencies, or released packaging; any broader user-facing MCP release story can remain with the umbrella MCP work under `#35294`. -- 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]
