terrymanu commented on PR #38872: URL: https://github.com/apache/shardingsphere/pull/38872#issuecomment-4743716068
### Summary - **Merge Decision: Mergeable** - **Reason:** The PR consistently removes the pseudo configurable LLM server runtime path and keeps the fixed `llama.cpp` runtime evidence without leaving unresolved scope, test, or docs gaps. ### Evidence - `LLME2EConfiguration` no longer reads, stores, or propagates `mcp.llm.server-runtime`; the remaining configuration still keeps runtime mode, image, base image, digest, and model metadata intact (`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/config/LLME2EConfiguration.java:51`, `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/config/LLME2EConfiguration.java:86`). - The workflow and local build script no longer emit or pass a `SERVER_RUNTIME` build argument, while the Dockerfile records the fixed runtime explicitly as `llama.cpp` (`.github/workflows/e2e-mcp.yml:107`, `.github/workflows/e2e-mcp.yml:122`, `test/e2e/mcp/src/test/resources/docker/llm-runtime/build-local.sh:96`, `test/e2e/mcp/src/test/resources/docker/llm-runtime/build-local.sh:140`, `test/e2e/mcp/src/test/resources/docker/llm-runtime/Dockerfile:30`). - Runtime evidence remains explicit for both paths: Docker-owned runtime reports `llama.cpp`, and external-debug runtime reports `external-openai-compatible` (`test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/fixture/LLMRuntimeSupport.java:179`, `test/e2e/mcp/src/test/java/org/apache/shardingsphere/test/e2e/mcp/llm/fixture/LLMRuntimeSupport.java:193`). - The changed scope is limited to MCP E2E configuration/tests, the MCP E2E workflow, and the test Docker runtime resources; it does not touch shared SQL parsing, Proxy/JDBC high-frequency execution paths, metadata/routing, dependency manifests, distribution packaging, or public runtime APIs. - A repository search on the PR head found no remaining `mcp.llm.server-runtime`, `SERVER_RUNTIME`, or `server_runtime` references in the changed workflow/resources; existing docs do not document the removed property. ### Review Details - **Reviewed Scope:** Latest PR head `0d2b66e6cf02c0aa685921164d50165a57052326`, base `master` at `291f612fb1004d09f7326e1470095b2b6ff06531`, merge-base `291f612fb1004d09f7326e1470095b2b6ff06531`. Reviewed all 10 GitHub `/pulls/38872/files` entries; local triple-dot file list matched GitHub. - **Not Reviewed Scope:** GitHub Actions status/check-runs were not inspected. Full Docker image build, model download, and full `*E2ETest` LLM workflow execution were not run locally because they are high-cost environment validation; the static build path, local helper dry-run, and focused test/style gates were reviewed instead. - **Verification:** `./mvnw -pl test/e2e/mcp -am -DskipITs -Dspotless.skip=true -Dtest=LLME2EConfigurationTest,LLMRuntimeSupportTest,LLMChatModelClientTest,LLMUsabilitySuiteRunnerTest -Dsurefire.failIfNoSpecifiedTests=false test -B -ntp` passed with exit code 0, 23 tests, and `BUILD SUCCESS`. `sh test/e2e/mcp/src/test/resources/docker/llm-runtime/build-local.sh --print` passed with exit code 0 and no `server_runtime` output. `./mvnw -pl test/e2e/mcp -am -DskipTests -DskipITs -Dspotless.skip=true checkstyle:check -Pcheck -T1C -B -ntp` passed with exit code 0. `./mvnw -pl test/e2e/mcp -am -DskipTests -DskipITs spotless:check -Pcheck -T1C -B -ntp` passed with exit code 0. - **Release Note / User Docs:** Not required. This is an MCP E2E test-runtime cleanup that removes an internal pseudo test property and restores the fixed runtime evidence; no released user configuration, API/SPI, SQL behavior, migration path, or published docs page needs an update. -- 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]
