terrymanu commented on PR #38830: URL: https://github.com/apache/shardingsphere/pull/38830#issuecomment-4647743975
### Summary - **Merge Decision: Mergeable** - **Reason:** The latest PR head is a narrow MCP markdown-loader rename with equivalent behavior, complete in-repo caller updates, focused regression coverage, and passing scoped verification. ### Evidence - `MCPMarkdownResourceLoader.load(...)` keeps the same resource-load contract: classpath lookup, Apache source-header stripping, blank-content rejection, missing-resource rejection, and unreadable-resource wrapping are preserved at `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/markdown/MCPMarkdownResourceLoader.java:51`. - All in-scope callers were updated to the renamed method in `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPPromptTemplateLoader.java:47`, `mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/server/MCPSyncServerFactory.java:82`, and `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/ProductionMySQLRuntimeE2ETest.java:140`; `git grep` on the PR head found no remaining `loadRequired` usage. - The regression tests still cover the important loader branches: success, fallback classloader, authored leading comment, missing resource, unreadable resource, and blank-after-license-header resource in `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/markdown/MCPMarkdownResourceLoaderTest.java:41`. - The shared support change remains generic MCP resource-loading behavior; it does not introduce target dialect names, routing/default-schema/name-resolution changes, parser changes, new hidden lifecycle state, or high-frequency `ConcurrentHashMap#computeIfAbsent` usage in the PR scope. - The linked issue `#35294` is an umbrella MCP product/design discussion; this PR does not claim to close the issue, and the narrow rename does not conflict with the MCP scope discussed there. - Release notes and user docs are not required for this PR because it does not change released user-facing MCP protocol behavior, configuration, SQL support, diagnostics, migration behavior, or documented workflows. ### Review Details - **Reviewed Scope:** Latest PR head `b6b8ba556effd055e57f3b53df24a4f123b5b8da`; base `eeaeecb6a095354eb0ac0735a1e93a62c61e8155`; local merge-base `eeaeecb6a095354eb0ac0735a1e93a62c61e8155`. Local `git diff --name-status <merge-base>..refs/remotes/apache/pr/38830` matched GitHub `/pulls/38830/files` for all 5 files: `mcp/bootstrap/src/main/java/org/apache/shardingsphere/mcp/bootstrap/transport/server/MCPSyncServerFactory.java`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/descriptor/MCPPromptTemplateLoader.java`, `mcp/support/src/main/java/org/apache/shardingsphere/mcp/support/markdown/MCPMarkdownResourceLoader.java`, `mcp/support/src/test/java/org/apache/shardingsphere/mcp/support/markdown/MCPMarkdownResourceLoaderTest.java`, and `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/runtime/production/ProductionMySQLRuntimeE2ETest.java`. - **Not Reviewed Scope:** GitHub Actions/check-run status by rule; full external MCP client interoperability; live production MySQL E2E execution beyond compilation and the focused loader unit test. - **Verification:** `./mvnw -pl mcp/support,mcp/bootstrap,test/e2e/mcp -am -DskipITs -Dspotless.skip=true -Dcheckstyle.skip=true -Dtest=MCPMarkdownResourceLoaderTest -Dsurefire.failIfNoSpecifiedTests=false test` exited 0. `./mvnw -pl mcp/support,mcp/bootstrap,test/e2e/mcp -am -Pcheck -DskipTests spotless:check checkstyle:check` exited 0. - **Release Note / User Docs:** Not required; the change is an internal MCP support utility rename with preserved behavior and no user-facing configuration, protocol, SQL, migration, or troubleshooting impact. -- 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]
