Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24908 )
Change subject: KUDU-3806: add the MCP server ...................................................................... Patch Set 1: (19 comments) Thank you for putting this together! I took a quick glance, posting a few notes. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/mcp_test_util.h File src/kudu/tools/mcp_test_util.h: http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/mcp_test_util.h@44 PS1, Line 44: // The parsed MCP tool result from a 'tools/call' JSON-RPC response. : struct ToolResult { : bool is_error; : std::string text; // content[0].text : }; This seems duplicated: there is another definition in tool_action_mcp.cc called ToolOutput, with the field order swapped. Is it intentional? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-itest.cc File src/kudu/tools/tool_action_mcp-itest.cc: http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-itest.cc@24 PS1, Line 24: #include <stdint.h> : #include <stdlib.h> : #include <sys/wait.h> : #include <unistd.h> nit: for some of these, find corresponding C++ counterparts and move into the section below http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-itest.cc@90 PS1, Line 90: struct ToolOutcome { : string text; : bool is_error; : }; Why to have this defined in multiple places? Is it possible to define it one common place and use everywhere? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-itest.cc@684 PS1, Line 684: EXPECT_NE(string::npos, tr.text.find("does not exist")) : << "expected a 'table does not exist' error for '" << flag_like : << "', got: " << tr.text; : EXPECT_NE(string::npos, tr.text.find(flag_like)) : << "error should echo the literal table name '" << flag_like : << "', got: " << tr.text; : // The child version banner must never appear: that would mean "--version" was : // parsed as a flag and the describe action never ran. : EXPECT_EQ(string::npos, tr.text.find("build type")) : << "child interpreted '" << flag_like << "' as a flag: " << tr.text; Any particular reason of using EXPECT_xxx instead of ASSERT_xxx when tr.is_error is handled with ASSERT_TRUE() in the same loop? Similar question elsewhere in this file regarding EXPECT vs ASSERT usage. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-test.cc File src/kudu/tools/tool_action_mcp-test.cc: http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-test.cc@299 PS1, Line 299: EXPECT_TRUE(Contains(names, "table_describe")) << line; : EXPECT_TRUE(Contains(names, "master_status")) << line; : EXPECT_FALSE(Contains(names, "table_delete")) << line; Any particular reason of using EXECT_xxx instead of ASSERT_xx here? Same for everywhere else in this file. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp-test.cc@411 PS1, Line 411: EXPECT_FALSE(Contains(ToolNames(RunToolsList(false)), "table_delete")); Why EXPECT_FALSE, not ASSERT_FALSE? Would the rest of the test case make sense if this triggers? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc File src/kudu/tools/tool_action_mcp.cc: http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@20 PS1, Line 20: #include <stdlib.h> nit: convert into '#include <cstdlib>' and move into the section below? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@93 PS1, Line 93: struct ToolOutcome { : string text; : bool is_error; : }; One more definition of the same entity? Why do we need so many? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@169 PS1, Line 169: return version == "2024-11-05" || : version == "2025-03-26" || : version == "2025-06-18"; This is how versions are defined in MCP? Wow, that's a piece of innovation, indeed. I guess none is backward-compatible with any other one, right? If so, then how this server can support all the three out of the box -- I didn't see any code that would change its behavior based on the version, but maybe I'm missing something? Also, 2025-03-26 doesn't match any version documented here: https://modelcontextprotocol.info/specification/ What's so special about 2025-03-26 that it's mentioned here? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@202 PS1, Line 202: // Bool/object/array are not valid JSON-RPC ids. Echo null to stay : // well-formed rather than propagating a nonsensical id. : jw->Null(); Isn't this a pure data loss? If so, wouldn't it make sense to track it somehow at least during development phase to understand that the generated JSON value isn't fit for MCP consumers? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@261 PS1, Line 261: & nit for here and elsewhere with lambda captures: is it possible to limit the scope only to 'protocol_version' or there is anything else that the lambda needs from the outside context? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@296 PS1, Line 296: flag_name.find("dry_run") != string::npos; Why this is handled differently from 'allow_writes' above? Both are supposed to be boolean flags, so noallow_writes and nodry_run should be treated similarly, no? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@427 PS1, Line 427: flag_type == "uint64" IIUC, this case cannot be properly handed by safe_strto64() and requires safe_strtou64(). http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@429 PS1, Line 429: safe_strto64(default_value, &v) Here and elsewhere safe_strtoxxx() is used: aren't we supposed to handle an error if safe_strto64() returns false? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1225 PS1, Line 1225: std:: nit for here and elsewhere: please use the same approach for STL classes that appear at least twice -- add corresponding 'using ...' directive in the beginning of the file and drop the 'std' namespace prefix http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1253 PS1, Line 1253: If the binary cannot be located we keep serving: : // HandleToolsCall turns an empty path into an isError tool result rather than : // taking the server down. What's the purpose of this strange behavior? Why to keep this nonsensical situation instead of reporting an error early and exiting, similar to failed flag validator or ValidateDispositionCoverageOrDie() above? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1270 PS1, Line 1270: \t\r\n There are two more characters commonly attributed to the "space" class in the ASCII character set: vertical tab '\v', form feed '\f'. http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1305 PS1, Line 1305: or a stream error I'm a bit concerned that unexpected stream errors might entail data loss and lead to hard-to-troubleshoot situations with this sloppy always-OK return approach. Why not to treat stream errors with non-OK return status? http://gerrit.cloudera.org:8080/#/c/24908/1/src/kudu/tools/tool_action_mcp.cc@1327 PS1, Line 1327: The serve-level master addresses Server-level master addresses? What are those and how are they different from client-level master addresses? -- To view, visit http://gerrit.cloudera.org:8080/24908 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59146a2fd4f5b8d61d3e494cc88974b396ed6856 Gerrit-Change-Number: 24908 Gerrit-PatchSet: 1 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yan-Daojiang <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 22 Sep 2026 03:26:59 +0000 Gerrit-HasComments: Yes
