terrymanu commented on PR #38832:
URL: https://github.com/apache/shardingsphere/pull/38832#issuecomment-4648857094
### Summary
- **Merge Decision: Mergeable**
- **Reason:** The latest PR head is scoped, the MCP completion providers and
descriptor targets are validated by focused tests and HTTP-path E2E coverage,
and no unresolved regression or ownership risk remains.
### Evidence
- The PR adds feature-owned MCP completion providers for readwrite-splitting
load-balance algorithms, shadow algorithms, sharding algorithms, and sharding
key generators; the implementations follow the existing encrypt/mask completion
pattern and only expose candidate value/label/source, not plugin property or
secret fields.
- Descriptor changes expose completion targets for the new feature plugin
resource references, plus prompt targets where the prompt descriptors declare
the completed argument; descriptor validator tests cover the advertised targets.
- Direct provider tests cover context type, supported references,
foreign-reference rejection, candidate filtering, DistSQL plugin query wiring,
and SPI registration for the new public provider classes.
- `HttpTransportCompletionE2ETest` exercises the real HTTP completion path
for readwrite-splitting, shadow, sharding algorithm, and sharding key-generator
plugin values.
- Side-effect scan: the patch does not change SQL parser behavior, routing,
metadata assembly, default schema/name resolution, config keys, public API/SPI
contracts outside the new MCP feature providers, dependencies, packaging,
LICENSE/NOTICE, or released protocol semantics.
- Performance scan: no Proxy/JDBC DML/DQL high-frequency execution path is
touched, and no `ConcurrentHashMap#computeIfAbsent` usage is introduced.
- Linked issue #35294 is a broad MCP ideation/design umbrella; this PR is a
partial MCP feature-completion slice and does not claim to close the whole
issue.
### Review Details
- **Reviewed Scope:** Latest PR head
`351db6633f6e21354439d6471a22f07d401029ff`; base and local merge-base
`9f7e21a810498f8080cb530ef9f6e33e5201ef4e`. Reviewed all 22 GitHub
`/pulls/38832/files` entries, and the local triple-dot diff file list matched
GitHub exactly. Scope covered
`mcp/features/{encrypt,mask,readwrite-splitting,shadow,sharding}` and
`test/e2e/mcp`.
- **Not Reviewed Scope:** CI/check-runs were not inspected. No SQL parser
dialect-family review was needed because no parser, grammar, visitor, or
dialect SQL syntax changed. No full-repo `clean install` or manual production
database/container smoke was run.
- **Verification:** `git fetch apache master
pull/38832/head:refs/remotes/apache/pr/38832` exited 0. `./mvnw -pl
mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding -am
-DskipITs -Dspotless.skip=true
-Dtest=ReadwriteSplittingLoadBalanceAlgorithmCompletionProviderTest,ShadowAlgorithmCompletionProviderTest,ShardingAlgorithmCompletionProviderTest
-Dsurefire.failIfNoSpecifiedTests=false test` exited 0. `./mvnw -pl
test/e2e/mcp -am -DskipITs -Dspotless.skip=true
-Dtest=HttpTransportCompletionE2ETest,MCPInteractionPayloadsTest
-Dsurefire.failIfNoSpecifiedTests=false test` exited 0. `./mvnw -pl
mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding,test/e2e/mcp
-am spotless:check -Pcheck -T1C -DskipTests` exited 0. `./mvnw -pl
mcp/features/readwrite-splitting,mcp/features/shadow,mcp/features/sharding,test/e2e/mcp
-am checkstyle:check -Pcheck -T1C -DskipTests` exited 0.
- **Release Note / User Docs:** Not required for this split PR; the change
extends MCP descriptor/provider completion discovery without migration,
compatibility, configuration, or upgrade action, and the existing MCP docs
already describe `completion/complete` and dynamic catalog/resource discovery.
--
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]
