terrymanu commented on PR #38914:
URL: https://github.com/apache/shardingsphere/pull/38914#issuecomment-4796001722
### Summary
**Review Result: Mergeable**
Reason: The latest PR head cleanly refactors MCP resource handler tests and
removes an unused sharding handler shim without losing registered resource
coverage. Scoped local tests, Checkstyle, and Spotless check passed.
### Evidence
-
`mcp/features/sharding/src/main/java/org/apache/shardingsphere/mcp/feature/sharding/ShardingMCPHandlerProvider.java:46`
registers the concrete sharding handler families directly at lines 48-69, so
the deleted `ShardingResourceHandler` shim is not on the production
registration path.
- A current repository search found no remaining `ShardingResourceHandler`
references outside generated `target` output.
- The replaced aggregate tests now have direct handler tests covering
context type, URI template, service dispatch, item payload, and navigation URI,
for example:
-
`mcp/features/broadcast/src/test/java/org/apache/shardingsphere/mcp/feature/broadcast/resource/handler/BroadcastTableRuleHandlerTest.java:39`
-
`mcp/features/encrypt/src/test/java/org/apache/shardingsphere/mcp/feature/encrypt/resource/handler/EncryptRuleHandlerTest.java:41`
-
`mcp/features/mask/src/test/java/org/apache/shardingsphere/mcp/feature/mask/resource/handler/MaskRuleHandlerTest.java:41`
-
`mcp/features/readwrite-splitting/src/test/java/org/apache/shardingsphere/mcp/feature/readwritesplitting/resource/handler/ReadwriteSplittingRuleStatusHandlerTest.java:40`
- The PR references `#35294` as an MCP umbrella issue but does not claim to
close it, so this cleanup does not need to satisfy the whole umbrella PRD scope.
### Review Details
- Reviewed Scope: PR `apache/shardingsphere#38914`, latest head
`b2d609080bec7069aa5faf7c7aaaf0a9b41da6c9`, base `master` at
`fed8b48fb30df7cd35c1b090a83a80feea51290f`, merge-base
`6b46d9854c4d58b4adaf10ce21faf78f1d33faed`. Reviewed all 21 GitHub changed
files; local triple-dot file list matched GitHub `/pulls/38914/files` after
normalizing GitHub `removed` to local `D`.
- Not Reviewed Scope: full repository build and local MCP Docker/E2E runtime
were not run. GitHub `E2E - MCP MySQL Runtime` was still in progress when
reviewed.
- Verification:
- Review inventory helper: exit 0.
- `./mvnw -pl
mcp/features/broadcast,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/sharding
-am -DskipITs -Dspotless.skip=true test`: exit 0, 172 tests, `BUILD SUCCESS`.
- `./mvnw -pl
mcp/features/broadcast,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/sharding
-am -DskipTests -Pcheck -T1C checkstyle:check`: exit 0, `BUILD SUCCESS`.
- `./mvnw -pl
mcp/features/broadcast,mcp/features/encrypt,mcp/features/mask,mcp/features/readwrite-splitting,mcp/features/sharding
-am -DskipTests -Pcheck -T1C spotless:check`: exit 0, `BUILD SUCCESS`.
- GitHub checks snapshot: `MCP - Unit Tests with JDK 21`, `Check -
Spotless`, `Check - CheckStyle`, and `Check - License` succeeded. The generic
`CI` job failed while fetching Maven `3.9.14` before project code ran, so I did
not classify it as PR-attributable.
- Release Note / User Docs: not required; this PR removes an unused internal
shim and refactors tests without changing public MCP resource URIs, user
behavior, configuration, dependencies, or distribution metadata.
--
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]
