-
Notifications
You must be signed in to change notification settings - Fork 161
Support persisting MCP tools in system index #3874
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
base: main
Are you sure you want to change the base?
Conversation
d50ddf6
to
e7e8369
Compare
...n/src/main/java/org/opensearch/ml/common/transport/mcpserver/requests/register/McpTools.java
Show resolved
Hide resolved
import lombok.extern.log4j.Log4j2; | ||
|
||
/** | ||
* This class represents a tool that can be registered with OpenSearch. It contains information about the tool's name, |
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.
I think we can make it clear by mentioning openSearch MCP Server.
Also can we use a better name for RegisterMcpTool
. This sounds like an action(function) rather than a class's name
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.
Renamed to McpToolRegsiterInput
for this and McpToolUpdateInput
for UpdateMcpTool.
...n/src/main/java/org/opensearch/ml/common/transport/mcpserver/requests/register/McpTools.java
Show resolved
Hide resolved
Instant lastUpdateTime | ||
) { | ||
super(name, type, description, parameters, attributes, createdTime, lastUpdateTime); | ||
if (type == null) { |
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.
Should we check for type first and then call the super method?
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.
super invocation should be in the first line of a constructor which is a java syntax limitation.
private static final String TYPE_NOT_SHOWN_EXCEPTION_MESSAGE = "type field required"; | ||
private Instant createdTime; | ||
private Instant lastUpdatedTime; | ||
public static final String TYPE_NOT_SHOWN_EXCEPTION_MESSAGE = "type field required"; |
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.
Should we move this variable to the RegisterBaseTool file if we are not using it here?
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.
Done
private Instant createdTime; | ||
private Instant lastUpdatedTime; | ||
public static final String TYPE_NOT_SHOWN_EXCEPTION_MESSAGE = "type field required"; | ||
public static final String NAME_NOT_SHOWN_EXCEPTION_MESSAGE = "name field required"; |
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.
Same as above, can we move to the update file?
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.
Done
@@ -0,0 +1,74 @@ | |||
package org.opensearch.ml.common.transport.mcpserver.requests.update; |
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.
Add the license header?
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.
Done
@@ -0,0 +1,45 @@ | |||
package org.opensearch.ml.common.transport.mcpserver.responses.list; |
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.
Add license header?
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.
Done
a60f924
to
1f6f359
Compare
import org.opensearch.action.ActionType; | ||
import org.opensearch.ml.common.transport.mcpserver.responses.remove.MLMcpToolsRemoveNodesResponse; | ||
|
||
public class MLMcpToolsRemoveAction extends ActionType<MLMcpToolsRemoveNodesResponse> { |
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.
Is that delete? Can we be consistent with other apis as well?
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.
Do you mean to rename the class or rename the API? In first release we named the API like below:
POST /_plugins/_ml/mcp/tools/_remove
[
"WebSearchTool", "ListIndexTool"
]
Signed-off-by: zane-neo <[email protected]>
Signed-off-by: zane-neo <[email protected]>
Signed-off-by: zane-neo <[email protected]>
Signed-off-by: zane-neo <[email protected]>
1f6f359
to
606a7c1
Compare
Signed-off-by: zane-neo <[email protected]>
Description
Support persisting MCP tools in system index in register API and adding new API, below are the new APIs specification:
Update MCP tools
Sample response
List MCP tools
Sample response
Related Issues
#3841
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.