terrymanu commented on PR #38749:
URL: https://github.com/apache/shardingsphere/pull/38749#issuecomment-4570696171
### Decision
- **Merge Verdict: Mergeable**
- **Reviewed Scope:** PR latest head
`d5e4998a7cee277dd10d4ff414757498a285d7f8`; base/merge-base
`8c1f9e0ffa9069cbb571e5aca5b887dac9923b26`. Reviewed all 47 files reported by
GitHub `/pulls/38749/files`; local triple-dot file list matched GitHub exactly.
Scope covers `distribution/mcp`,
`docs/document/content/user-manual/shardingsphere-mcp`, `mcp/bootstrap`,
`mcp/core` tests, `mcp/registry` test, `mcp/server.json`, and `mcp/support`.
- **Not Reviewed Scope:** GitHub Actions / CI status was not reviewed. I did
not run a live ShardingSphere-Proxy/MySQL environment smoke test; the review
used package verification, source inspection, residual searches, and targeted
unit tests.
- **Need Expert Review:** No additional security, concurrency, performance,
or SQL parser expert review is required for this change set.
### Basis
- The PR directly addresses the stated scope: it removes the embedded H2
demo SQL, H2 runtime dependency, generated `data` layout, H2 MCP capability SPI
registration, and H2 metadata/profile handling from the MCP path.
- The default MCP distribution now points users to a user-prepared
ShardingSphere-Proxy/MySQL logical database instead of a project-provided demo
database, as shown in
`distribution/mcp/src/main/resources/conf/mcp-http.yaml:19` and
`distribution/mcp/src/main/resources/conf/mcp-stdio.yaml:19`.
- The distribution dependency set keeps MySQL as the packaged driver and no
longer declares H2 in the MCP distribution dependency block; the runtime layout
now creates only `plugins/` and `logs/` at `distribution/mcp/pom.xml:54` and
`distribution/mcp/pom.xml:140`.
- The documentation now clearly says the quick start requires a
user-prepared ShardingSphere-Proxy logical database and placeholder
table/account values, for example
`docs/document/content/user-manual/shardingsphere-mcp/quick-start.cn.md:6`.
- The shared MCP metadata path remains constrained to supported non-H2
dialects. Database type detection still handles MySQL, PostgreSQL, openGauss,
SQLServer, MariaDB, Oracle, and Firebird paths in
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/metadata/jdbc/MCPJdbcDatabaseProfileLoader.java:115`.
- Sequence metadata behavior remains covered for dialects that still support
it, and H2 is intentionally absent from the sequence matrix in
`mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/database/metadata/jdbc/MCPJdbcMetadataLoader.java:164`
and
`mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/database/metadata/jdbc/MCPJdbcMetadataLoaderTest.java:386`.
- Fresh adversarial pass did not find unresolved risk:
- Cross-dialect path: non-H2 capability matrix and metadata tests still
cover MySQL, PostgreSQL, openGauss, SQLServer, MariaDB, Oracle, Doris, Hive,
Presto, Firebird, and related sequence behavior.
- Config-disabled / feature-flag path: no feature flag or disabled branch
is introduced; both HTTP and STDIO configuration parsing paths are covered.
- Original symptom path: residual source/document search and packaged
artifact inspection both show no H2 demo/runtime remnants in MCP scope.
- No unrelated changes were found; all touched files are MCP code, MCP
distribution assets, MCP tests, or MCP user manual pages.
### Verification
- `curl` GitHub PR metadata and `/pulls/38749/files`: 47 files; matched
local `git diff $(git merge-base refs/remotes/apache/master
refs/remotes/apache/pr/38749)..refs/remotes/apache/pr/38749`.
- `./mvnw -pl mcp/support -am -Dspotless.skip=true -DskipITs -DskipITs=true
-Dtest=MCPDatabaseCapabilityProviderTest,MCPJdbcMetadataLoaderTest,RuntimeDatabaseConfigurationTest
-Dsurefire.failIfNoSpecifiedTests=false test`: passed, 52 tests.
- `./mvnw -pl mcp/core,mcp/bootstrap,mcp/registry -am -Dspotless.skip=true
-DskipITs -DskipITs=true
-Dtest=MCPJdbcStatementExecutorTest,MCPSQLExecutionFacadeConcurrencyTest,SearchMetadataToolServiceTest,MCPRuntimeLauncherTest,MCPRegistryMetadataCommandTest,MCPLaunchConfigurationTest,MCPConfigurationLoaderTest,YamlMCPLaunchConfigurationSwapperTest,YamlMCPTransportConfigurationSwapperTest,YamlRuntimeDatabaseConfigurationSwapperTest,YamlRuntimeDatabaseConfigurationsSwapperTest
-Dsurefire.failIfNoSpecifiedTests=false test`: passed, affected MCP tests
passed.
- `./mvnw -pl distribution/mcp -am -DskipTests -Dspotless.skip=true
package`: passed.
- Packaged artifact check under
`distribution/mcp/target/apache-shardingsphere-mcp-5.5.4-SNAPSHOT`: no H2 jar,
no `data` directory, no `demo-h2.sql`; `mysql-connector-j-8.4.0.jar` is present.
- Residual search for H2/demo/runtime wording in `mcp`, `distribution/mcp`,
and MCP user manual docs returned no matches.
- `./mvnw spotless:check -Pcheck -T1C`: passed.
- `./mvnw checkstyle:check -Pcheck -T1C`: passed.
--
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]
