[ISSUE #13526] Add MCP indexing cache support. #13604
Conversation
|
|
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
添加MCP索引缓存支持及配置管理功能变更概述新功能
性能优化
测试更新
重构
配置调整
变更文件
时序图sequenceDiagram
participant Client as MCP客户端
participant Cache as MemoryMcpCacheIndex
participant DB as 数据库查询层
participant Sync as 缓存同步任务
Client->>Cache: getMcpServerById(mcpId)
Cache->>Cache: 检查缓存是否存在
Cache-->>Client: 返回缓存数据（命中时）
Cache--xClient: 缓存未命中时
Client->>DB: 执行数据库查询
DB-->>Client: 返回数据库结果
Client->>Cache: 更新缓存数据
Cache-->>Client: 返回最新数据
Sync->>DB: 定期全量同步任务
DB-->>Cache: 提交同步数据
Cache->>Cache: 更新本地缓存
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 1 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 1 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 3 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 5 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 10 个问题
⚠️ 需要立即关注的阻断性问题
ai/src/main/java/com/alibaba/nacos/ai/index/PlainMcpServerIndex.java
- 缺少Spring组件注解，导致无法被正确注入。 (L23)
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ ai/src/main/java/com/alibaba/nacos/ai/index/CachedMcpServerIndex.java (3 💬)
- 缓存配置属性获取方式存在不一致，可能导致配置值不一致问题 (L77-L78)
- 缓存同步任务未提供关闭机制可能导致资源泄漏 (L266-L280)
- 日志格式字符串使用不兼容的占位符格式 (L309)
☕ ai/src/main/java/com/alibaba/nacos/ai/index/MemoryMcpCacheIndex.java (3 💬)
- 未清理过期的nameKeyToId条目，导致缓存不一致。 (L72-L84)
- 日志格式字符串错误，导致统计信息无法正确输出。 (L201-L203)
- 未实现缓存项的过期检查逻辑，导致缓存无法自动清理。 (L228-L231)
☕ ai/src/main/java/com/alibaba/nacos/ai/index/PlainMcpServerIndex.java (1 💬)
- 缺少Spring组件注解，导致无法被正确注入。 (L23)
☕ ai/src/test/java/com/alibaba/nacos/ai/index/CachedMcpServerIndexTest.java (1 💬)
- 系统属性修改未在测试后重置 (L77-L79)
☕ ai/src/test/java/com/alibaba/nacos/ai/index/McpCachePerformanceTest.java (1 💬)
- 并发更新测试生成的mcpName/mcpId超出缓存容量导致测试数据无效 (L172-L173)
☕ ai/src/test/java/com/alibaba/nacos/ai/index/MemoryMcpCacheIndexTest.java (1 💬)
- 空参数测试未验证缓存容量限制 (L168-L169)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 配置属性获取方式不一致导致潜在配置冲突
在McpServerIndexConfiguration配置类中，缓存配置属性通过@value注解读取，而在CachedMcpServerIndex构造函数中，syncInterval参数却通过System.getProperty获取。这种不一致的配置获取方式可能导致配置值不一致，例如当系统属性与Spring配置文件配置不同时，会出现不可预测的行为。应统一使用Spring的依赖注入方式获取所有配置属性。
📌 关键代码
this.syncInterval = Long.parseLong(System.getProperty("nacos.mcp.cache.sync.interval", "300"));@Value("${nacos.mcp.cache.sync-interval-seconds:300}")
private long syncIntervalSeconds;配置值不一致可能导致缓存同步任务执行频率与预期不符，引发缓存数据不一致或资源浪费问题
🔍2. 缓存过期逻辑未实现导致内存泄漏风险
MemoryMcpCacheIndex的Entry类中isExpired()方法始终返回false，未实现基于expireTimeSeconds的过期检查逻辑。虽然配置了缓存过期时间，但实际未生效，导致缓存项永久驻留内存，可能引发内存泄漏风险。
📌 关键代码
boolean isExpired() {
return false;
}未清理过期缓存项将导致内存占用持续增长，最终引发系统OOM错误
🔍3. 缓存同步线程池未正确关闭
CachedMcpServerIndex中的ScheduledExecutorService未在Spring应用上下文关闭时正确关闭。虽然syncTask任务可被取消，但线程池本身未实现关闭逻辑，可能导致资源泄漏。
📌 关键代码
public ScheduledExecutorService mcpCacheScheduledExecutor() {
return new ScheduledThreadPoolExecutor(...);
}线程池未关闭可能导致进程退出时残留线程，影响系统资源回收和稳定性
🔍4. 核心搜索方法未使用缓存导致性能浪费
CachedMcpServerIndex的searchMcpServerByName方法在缓存开启时直接返回数据库查询结果，未利用缓存加速。该方法注释提到'优先查询缓存'但实际未实现缓存预热逻辑，完全绕过了缓存机制。
📌 关键代码
if (cacheEnabled) {
// Priority query from cache (only hit statistics here, actual pagination still queries database)
return searchFromDatabase(...)
}缓存未被有效利用，导致高频查询直接冲击数据库，无法发挥缓存应有的性能优化作用
🔍5. 并发性能测试生成数据超出缓存容量
McpCachePerformanceTest的并发测试中，当THREAD_COUNT=10且OPERATION_COUNT=10000时，总写入量达10万次，远超配置的CACHE_SIZE=1000。这会导致大量缓存淘汰，测试结果无法准确反映正常负载下的性能表现。
📌 关键代码
private static final int THREAD_COUNT = 10;
private static final int OPERATION_COUNT = 10000;
private static final int CACHE_SIZE = 1000;测试数据设计不合理可能导致测试结果偏差，无法真实反映缓存在合理容量下的性能表现
🔍6. 缓存清理任务未实现自动过期
MemoryMcpCacheIndex未实现基于expireTimeSeconds的自动过期清理机制，仅依赖LRU淘汰策略。当缓存项存活时间超过配置的expireTimeSeconds时，无效数据仍可能长期占用内存空间。
📌 关键代码
this.expireTimeSeconds = expireTimeSeconds;无法及时清理过期数据将导致缓存污染，影响查询准确性并加剧内存压力
🔍7. 缓存统计指标未包含过期淘汰次数
CacheStats类中缺少evictionCount统计项的实际计算逻辑。虽然定义了evictionCount字段，但现有代码未在LRU淘汰时更新该计数器，导致统计信息不完整。
📌 关键代码
private final AtomicLong evictionCount = new AtomicLong();无法准确监控缓存淘汰事件，影响系统性能调优和问题诊断
🔍8. 核心接口方法缺乏参数有效性校验
McpCacheIndex接口的updateIndex方法未在接口层定义参数有效性约束，导致实现类需要重复处理空值校验。应在接口方法文档中明确参数约束，避免多处重复校验逻辑。
📌 关键代码
void updateIndex(String namespaceId, String mcpName, String mcpId);参数校验分散可能导致部分实现遗漏校验，引发空指针异常或无效数据写入
审查详情
📒 文件清单 （10 个文件）
✅ 新增: 8 个文件
📝 变更: 2 个文件
✅ 新增文件:
ai/src/main/java/com/alibaba/nacos/ai/config/McpServerIndexConfiguration.javaai/src/main/java/com/alibaba/nacos/ai/index/CachedMcpServerIndex.javaai/src/main/java/com/alibaba/nacos/ai/index/McpCacheIndex.javaai/src/main/java/com/alibaba/nacos/ai/index/MemoryMcpCacheIndex.javaai/src/test/java/com/alibaba/nacos/ai/config/McpServerIndexConfigurationTest.javaai/src/test/java/com/alibaba/nacos/ai/index/CachedMcpServerIndexTest.javaai/src/test/java/com/alibaba/nacos/ai/index/McpCachePerformanceTest.javaai/src/test/java/com/alibaba/nacos/ai/index/MemoryMcpCacheIndexTest.java
📝 变更文件:
ai/src/main/java/com/alibaba/nacos/ai/index/McpServerIndex.javaai/src/main/java/com/alibaba/nacos/ai/index/PlainMcpServerIndex.java
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
ai/src/main/java/com/alibaba/nacos/ai/index/CachedMcpServerIndex.java
Outdated
Show resolved
Hide resolved
ai/src/main/java/com/alibaba/nacos/ai/index/CachedMcpServerIndex.java
Outdated
Show resolved
Hide resolved
ai/src/main/java/com/alibaba/nacos/ai/index/MemoryMcpCacheIndex.java
Outdated
Show resolved
Hide resolved
ai/src/main/java/com/alibaba/nacos/ai/index/MemoryMcpCacheIndex.java
Outdated
Show resolved
Hide resolved
ai/src/test/java/com/alibaba/nacos/ai/index/CachedMcpServerIndexTest.java
Show resolved
Hide resolved
ai/src/test/java/com/alibaba/nacos/ai/index/McpCachePerformanceTest.java
Outdated
Show resolved
Hide resolved
| // Test update operations with null parameters (should not throw exceptions) | ||
| assertDoesNotThrow(() -> cacheIndex.updateIndex(null, "test", "id")); |
There was a problem hiding this comment.
空参数测试未验证缓存容量限制
🟢 Minor | 🧹 Code Smells
📋 问题详情
在testNullAndEmptyParameters测试中，未验证更新空参数是否会影响缓存容量统计，可能导致缓存统计逻辑缺陷。
💡 解决方案
添加容量验证断言：
@@ -168,2 +168,4 @@
168 assertDoesNotThrow(() -> cacheIndex.updateIndex("test", null, "id"));
169 assertDoesNotThrow(() -> cacheIndex.updateIndex("test", "name", null));
+
+ assertEquals(0, cacheIndex.getSize(), "空参数更新不应占用缓存空间");您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
|
@lingma-agents Regarding your question about removing the |
|
这个改动的实现是合理的，通过配置类统一管理Bean创建有以下优势：
关键验证点：
补充说明：
这种设计模式符合Spring框架的最佳实践，通过条件化配置实现了灵活的组件切换，同时保持了代码的可测试性和可维护性。 |
@lingma-agents So does your Code Review Report need to be re-optimized and tweaked a bit? |
|
您的实现设计合理且符合Spring最佳实践，以下是对优化方向的详细分析：
|
KomachiSion
left a comment
There was a problem hiding this comment.
I have some confused about McpCachedIndex and McpServerIndex
I think the McpCachedIndex is a implementation of McpServerIndex .
But I see you abstract and new McpCachedIndex which be covered and proxyed by CachedMcpServerIndex of implementation of McpServerIndex.
What's the diff about McpCachedIndex and McpServerIndex in design?
| @Value("${nacos.mcp.cache.expire-time-seconds:3600}") | ||
| private long expireTimeSeconds; | ||
|
|
||
| @Value("${nacos.mcp.cache.cleanup-interval-seconds:300}") |
There was a problem hiding this comment.
default 300 ms whether too fast and frequently？
There was a problem hiding this comment.
This part of the code is still being optimized and needs some more suggested values based on the results of the benchmarking.
| @Value("${nacos.mcp.cache.cleanup-interval-seconds:300}") | ||
| private long cleanupIntervalSeconds; | ||
|
|
||
| @Value("${nacos.mcp.cache.sync-interval-seconds:300}") |
There was a problem hiding this comment.
default 300 ms whether too fast and frequently？
| * | ||
| * @author misselvexu | ||
| */ | ||
| public interface McpCacheIndex { |
There was a problem hiding this comment.
What's the diffenent with McpServerIndex?
There was a problem hiding this comment.
See specifically the response below, "MCP Server Index Architecture - Core Relationships".
MCP Server Index Architecture - Core RelationshipsInterface HierarchyComponent ResponsibilitiesMcpServerIndex - Core Business Interfacepublic interface McpServerIndex {
Page<McpServerIndexData> searchMcpServerByName(...);
McpServerIndexData getMcpServerById(String id);
McpServerIndexData getMcpServerByName(String namespaceId, String name);
}
McpCacheIndex - Cache Strategy Interfacepublic interface McpCacheIndex {
String getMcpId(String namespaceId, String mcpName);
McpServerIndexData getMcpServerById(String mcpId);
void updateIndex(String namespaceId, String mcpName, String mcpId);
void removeIndex(String mcpId);
CacheStats getStats();
}
Implementation RelationshipsPlainMcpServerIndex - No Cachepublic class PlainMcpServerIndex implements McpServerIndex {
// Direct database queries - no caching
public McpServerIndexData getMcpServerById(String id) {
// Query database directly for each request
return queryDatabase(id);
}
}
CachedMcpServerIndex - Cache Decoratorpublic class CachedMcpServerIndex implements McpServerIndex {
private final McpCacheIndex cacheIndex; // Composition
public McpServerIndexData getMcpServerById(String id) {
// 1. Try cache first
McpServerIndexData cached = cacheIndex.getMcpServerById(id);
if (cached != null) return cached;
// 2. Fallback to database
McpServerIndexData dbData = queryDatabase(id);
if (dbData != null) {
cacheIndex.updateIndex(dbData.getNamespaceId(), dbData.getId(), dbData.getId());
}
return dbData;
}
}
MemoryMcpCacheIndex - Cache Implementationpublic class MemoryMcpCacheIndex implements McpCacheIndex {
private final ConcurrentHashMap<String, CacheNode> idToEntry;
private final ConcurrentHashMap<String, String> nameKeyToId;
// LRU eviction, expiration, statistics
}
Configuration-Driven Selection@Configuration
public class McpServerIndexConfiguration {
@Bean
@ConditionalOnProperty(name = "nacos.mcp.cache.enabled", havingValue = "true")
public McpServerIndex cachedMcpServerIndex(McpCacheIndex cacheIndex, ...) {
return new CachedMcpServerIndex(..., cacheIndex, ...);
}
@Bean
@ConditionalOnProperty(name = "nacos.mcp.cache.enabled", havingValue = "false")
public McpServerIndex plainMcpServerIndex(...) {
return new PlainMcpServerIndex(...);
}
}Key Design Patterns
Flow SummaryCore Principle: Business logic ( |
|
OK, I will merged this PR after CI passed. @misselvexu CI can't pass, please fix. |
Hold on, I'll fix it right away. |
|
|
|
|
|
UT problem should be fixed by develop branch, maybe you can merged develop branch and retry. |
There is something odd about the fact that I utilize the commands in our ci to do UT in the local environment. Mainly because I haven't changed or tweaked the code for the Nacos Console module either yay! Ci Error (nacos-console) module : Command: Output: 🤔🤔🤔 |
|
It might be diff in some unit test order. In github action env, ConsoleExceptionHandlerTest maybe run with high order and without Environment so that problem occus. Develop branch has enhance it, I suggest you merged develop branch. |
|
Error: Failures: |
|
@misselvexu Thanks for contribution. Do you have dingtalk number? I would like invite you into nacos constribution group. |
|
|
Dingtalk number is |
|
|
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
Fixes #13526
This PR adds MCP (Model Context Protocol) indexing cache support to improve performance when handling MCP server index operations. The implementation provides a configurable caching mechanism that can be enabled/disabled via the
nacos.mcp.cache.enabledproperty.Brief changelog
CachedMcpServerIndeximplementation with configurable caching supportMcpServerIndexConfigurationfor unified conditional bean creationMcpCacheIndexfor managing cached MCP server dataPlainMcpServerIndexto remove@Serviceannotation to avoid bean conflictsnacos.mcp.cache.enabledto control caching behaviorVerifying this change
Unit Tests
McpServerIndexConfigurationTestto verify conditional bean creationCachedMcpServerIndexTestto verify caching behaviorMcpCacheIndexTestto verify cache management functionalityManual Testing
Cache Enabled Mode (
nacos.mcp.cache.enabled=true):CachedMcpServerIndexis injected as the primary implementationCache Disabled Mode (
nacos.mcp.cache.enabled=false):PlainMcpServerIndexis injected as the fallback implementationPerformance Testing
Configuration Testing
trueandfalsevalues fornacos.mcp.cache.enabledwork correctlyChecklist
[ISSUE #123] Fix UnknownException when host config not exist. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean package apache-rat:check findbugs:findbugs -Dmaven.test.skip=trueto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.Additional Notes
McpServerIndexConfigurationfor better maintainability