-
Notifications
You must be signed in to change notification settings - Fork 121
feat: support mcp async toolset #306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support mcp async toolset #306
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @wenhaozhao, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the ADK framework's ability to interact with external Model Context Protocol (MCP) servers by introducing a new asynchronous toolset. This new McpAsyncToolset
facilitates the dynamic discovery and integration of tools from an MCP server, complete with robust error handling and retry mechanisms. Additionally, a new utility for filtering tools by name has been added, and the existing MCP-related exceptions have been refactored into a dedicated file for better code structure.
Highlights
- Asynchronous MCP Toolset: I've introduced
McpAsyncToolset
, a new component designed to asynchronously connect to an MCP (Model Context Protocol) server, retrieve available tools, and integrate them into the ADK framework. This toolset supports configurable connection parameters and dynamic tool filtering, including robust retry mechanisms for tool loading. - Tool Filtering Utility: I've added
NamedToolPredicate
, a newToolPredicate
implementation that allows filteringBaseTool
instances based on a predefined list of tool names. This provides a convenient and reusable way to select specific tools from a larger set. - Exception Refactoring: I've refactored the exception hierarchy for MCP-related operations by moving
McpToolsetException
,McpInitializationException
, andMcpToolLoadingException
into their own dedicated file,McpToolsetException.java
. This improves the organization and clarity of exception handling within the MCP module.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces an asynchronous version of the MCP toolset, McpAsyncToolset
, which uses Project Reactor and RxJava for non-blocking operations. It also adds a NamedToolPredicate
for filtering tools by name and refactors the MCP toolset exceptions into their own file for better organization.
The review focuses on the new McpAsyncToolset
implementation. I've identified a potential high-severity concurrency issue in the retry logic that could affect applications sharing a toolset instance across multiple threads. I've also suggested a couple of medium-severity improvements to enhance code clarity and maintainability by using more idiomatic constructs for session creation and tool filtering.
if (totalRetries < MAX_RETRIES) { | ||
logger.info( | ||
"Reinitializing MCP session before next retry for unexpected error."); | ||
this.close(); | ||
return Mono.just(err) | ||
.delayElement(Duration.ofMillis(RETRY_DELAY_MILLIS)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current retry logic calls this.close()
, which atomically retrieves and closes the shared mcpSession
. This can cause serious issues in a multi-threaded environment where a single McpAsyncToolset
instance is shared among concurrent operations.
If one thread's call to getTools()
fails and enters the retry logic, it will close the session that other concurrent threads might be actively using. This will likely cause the operations of other threads to fail unexpectedly.
To address this, you should either implement a more sophisticated retry mechanism that doesn't interfere with other threads or clearly document that a single McpAsyncToolset
instance is not safe for concurrent calls to getTools()
.
this.mcpSession.accumulateAndGet( | ||
null, | ||
(prev, _next) -> { | ||
// set if not present | ||
if (prev == null) { | ||
return this.mcpSessionManager.createAsyncSession(); | ||
} | ||
return prev; | ||
})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of accumulateAndGet
for a "get or create" operation is a bit unconventional. The _next
parameter is unused, which suggests that a different method might be more idiomatic.
Using updateAndGet
more clearly expresses the intent to atomically update the reference, making the code easier to understand and maintain.
this.mcpSession.updateAndGet(
prev -> {
// set if not present
if (prev == null) {
return this.mcpSessionManager.createAsyncSession();
}
return prev;
})
.filter( | ||
tool -> | ||
isToolSelected( | ||
tool, | ||
Optional.of(toolFilter), | ||
Optional.ofNullable(readonlyContext))) | ||
.collect(toImmutableList()))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filtering logic is more complex than necessary because it calls the generic isToolSelected
default method from the BaseToolset
interface.
Since the McpAsyncToolset.Builder
ensures that toolFilter
is always a non-null ToolPredicate
, you can simplify this by calling the test
method on toolFilter
directly. This makes the code more direct and easier to understand.
.filter(
tool ->
toolFilter.test(
tool, Optional.ofNullable(readonlyContext)))
.collect(toImmutableList())
89b57ab
to
38d57db
Compare
38d57db
to
b867ea2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Internal CI/CD failed due to the reactor core dependency missing from our internal build. Upon closer look, could you share a bit why the choice of Mono over the RxJava types used across the rest of codebase (Single, Maybe, Flowable, etc.)? I'd like to verify whether this is critical prior to adding the dep
Reactor is a required dep by Mcp-java-sdk
reactor-core is a required dep by mcp-java-sdk see https://github.com/modelcontextprotocol/java-sdk/blob/v0.10.0/mcp/pom.xml ![]() |
Fixed the missing dep internally, going to retry merging |
Support McpAsyncToolset for McpAsyncTool