[ISSUE#13449] Fix service pagination #13459
Conversation
|
|
修复服务分页逻辑并添加测试用例变更文件
时序图sequenceDiagram
participant CS as CatalogServiceV2Impl
participant SSM as ServiceStorage
participant NMM as NamingMetadataManager
CS->>SSM: 获取服务实例列表
SSM-->>CS: 返回服务实例集合
CS->>NMM: 查询服务元数据
NMM-->>CS: 返回元数据对象
CS->>CS: 计算分页参数
CS->>CS: 验证保护阈值条件
CS-->>客户端: 返回分页结果
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 0 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 0 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 1 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 1 个问题
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ naming/src/test/java/com/alibaba/nacos/naming/core/CatalogServiceV2ImplTest.java (1 💬)
- 补充`testListService`测试用例的断言验证 (L184-L201)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. 分页逻辑接口不一致引入潜在兼容性风险
新增的pageListService与listService方法在分页实现上存在逻辑重复且返回类型不一致。pageListService返回ObjectNode而listService返回Page，这种不一致可能导致客户端调用混乱。需要统一分页接口的设计规范，避免同一业务场景出现多个并行实现。
📌 关键代码：
public Object pageListService(String namespaceId, String groupName, String serviceName, int pageNo, int pageSize, String instancePattern, boolean ignoreEmptyService)public Page<ServiceView> listService(String namespaceId, String groupName, String serviceName, int pageNo, int pageSize, boolean ignoreEmptyService)🔍 2. 集群信息收集存在性能隐患
getClusterMap方法通过遍历所有实例来收集集群信息，当服务实例数量庞大时会导致O(n)时间复杂度。建议引入缓存机制或预计算集群元数据，避免在每次请求时进行全量计算。
📌 关键代码：
for (Instance each : serviceStorage.getData(service).getHosts()) { ... }🔍 3. 分页测试覆盖不完整
新增的listService测试用例仅验证了基础分页逻辑，未覆盖分页参数边界值（如页码为0、pageSize超过总记录数）和过滤条件组合场景。建议补充极端值测试和模式匹配测试用例。
📌 关键代码：
@Test void testListService() { ... }🔍 4. 服务元数据访问存在设计缺陷
getServiceDetail方法直接通过ServiceMetadata::new创建默认值，但未校验基础服务是否存在。当服务实例不存在时应提前终止流程，避免产生无效元数据对象。
📌 关键代码：
ServiceMetadata::new🔍 5. 保护阈值计算存在精度风险
isProtectThreshold方法使用float类型进行保护阈值计算，存在浮点精度误差风险。建议改用BigDecimal或整数百分比表示，避免因精度问题导致策略判断失误。
📌 关键代码：
metadata.getProtectThreshold()💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
| void testListService() throws NacosException { | ||
| ServiceInfo serviceInfo = new ServiceInfo(); | ||
| Mockito.when(serviceStorage.getData(Mockito.any())).thenReturn(serviceInfo); | ||
| ServiceManager.getInstance().getSingleton(Service.newService("CatalogService", "CatalogService", "1")); | ||
| ServiceManager.getInstance().getSingleton(Service.newService("CatalogService", "CatalogService", "2")); | ||
| ServiceManager.getInstance().getSingleton(Service.newService("CatalogService", "CatalogService", "3")); | ||
| Page<ServiceView> result = catalogServiceV2Impl.listService("CatalogService", "", "", 2, 1, false); | ||
|
|
||
| assertNotNull(result); | ||
| assertEquals(3, result.getTotalCount(), "Total service count should be 3"); | ||
| assertEquals(2, result.getPageNumber(), "Current page number should be 2"); | ||
| assertEquals(4, result.getPagesAvailable(), "PagesAvailable should = (totalCount / pageSize) + 1"); | ||
| assertEquals(1, result.getPageItems().size(), "Page size is 1, so only one item should be returned"); | ||
|
|
||
| ServiceView serviceView = result.getPageItems().get(0); | ||
| assertEquals(serviceView.getName(), "2", "Service name should be '2' "); | ||
| assertEquals("CatalogService", serviceView.getGroupName(), "Group name should be CatalogService"); | ||
| } |
There was a problem hiding this comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #13459 +/- ##
=============================================
+ Coverage 70.52% 70.57% +0.04%
- Complexity 11912 11914 +2
=============================================
Files 1621 1621
Lines 51826 51826
Branches 5210 5210
=============================================
+ Hits 36552 36575 +23
+ Misses 12826 12801 -25
- Partials 2448 2450 +2
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
@OmCheeLin Please sign cla first |
ok, I have done it. |
| @@ -55,18 +55,18 @@ | |||
| */ | |||
| @Component() | |||
| public class CatalogServiceV2Impl implements CatalogService { | |||
|
|
|||
There was a problem hiding this comment.
Please dont change the indent
|
The only problem is the indent changed, please setting the indent and reformat these two files. |
|
|
@OmCheeLin Hello, please fix the indent. If still no response, PR will be closed. |
修复服务分页逻辑并增强健康实例统计功能变更文件
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
ok，I will repair today |
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
There was a problem hiding this comment.
🔍 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 1 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 1 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 1 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 1 | 次要问题，酌情优化。例如：代码格式不规范或注释缺失。 |
总计： 4 个问题
⚠️ **需要立即关注的阻断性问题**
naming/src/main/java/com/alibaba/nacos/naming/core/CatalogServiceV2Impl.java
- 修正分页计算逻辑中的变量名错误 (L175)
📋 评审意见详情
💡 单文件建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ naming/src/main/java/com/alibaba/nacos/naming/core/CatalogServiceV2Impl.java (3 💬)
- 添加分页参数的有效性校验 (L135)
- 修正分页计算逻辑中的变量名错误 (L175)
- 修正正则表达式构造逻辑 (L257-L262)
☕ naming/src/test/java/com/alibaba/nacos/naming/core/CatalogServiceV2ImplTest.java (1 💬)
- 修正分页测试用例的断言逻辑错误 (L194)
🚀 跨文件建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. 分页接口返回类型不一致导致调用混乱
在CatalogServiceV2Impl中同时存在返回ObjectNode的pageListService和返回Page对象的listService方法，两种分页接口返回类型不一致。这会导致调用方需要针对不同接口处理不同的数据结构，增加客户端适配成本。建议统一使用Page对象作为分页结果的标准返回类型，避免接口碎片化。
📌 关键代码：
public Object pageListService(...){}
public Page<ServiceView> listService(...){}- 客户端需要维护两种不同的数据解析逻辑
- 新开发人员容易混淆接口使用场景
- 后续维护时可能引入类型转换错误
🔍 2. 分页核心逻辑未统一抽象
分页处理逻辑分散在doPage、listService、pageListService等多个方法中，核心分页逻辑（如页码计算、分页切片）未进行统一抽象。例如doPage方法直接操作集合分页，而pageListService又包含额外的过滤逻辑，这种分散实现增加了维护复杂度。
📌 关键代码：
private Collection<Service> doPage(Collection<Service> services, int pageNo, int pageSize) {
if (pageNo == 0 && services.size() < pageSize) {
return services;
}- 分页参数处理不一致（如页码起始值、边界值处理）
- 新增分页接口时需要重复实现基础逻辑
- 调试时难以追踪分页逻辑的完整执行路径
🔍 3. 分页参数有效性校验不全面
虽然在CatalogServiceV2Impl.java第135行添加了分页参数校验，但测试用例未覆盖页码和页大小的负值场景。此外，当pageSize为0时存在潜在除零风险（如计算PagesAvailable时），需补充相关校验逻辑。
📌 关键代码：
ObjectNode obj = ... pageListService("CatalogService", "", "", 2, 1, ...)- 系统出现ArithmeticException异常
- 返回无效的分页信息（如页数计算错误）
- 恶意请求引发服务异常
🔍 4. 分页性能隐患
doPage方法采用内存分页方式处理服务列表，当服务数量达到百万级时会导致内存压力和性能下降。例如在测试用例中创建3个服务时即需要加载全部数据到内存，未使用数据库原生分页能力。
📌 关键代码：
private Collection<Service> doPage(Collection<Service> services, ...) {
if (pageNo == 0 && services.size() < pageSize) {
return services;
}- 高负载时内存溢出
- 查询响应时间随数据量增长线性增加
- 数据库连接池因全量查询被阻塞
🔍 5. 测试覆盖不充分
新增的listService测试用例（第184行）仅验证了正常分页场景，未覆盖以下关键场景：
- 页码超过最大页数时的返回
- 服务名模式匹配与分页的联合过滤
- ignoreEmptyService参数对分页结果的影响
📌 关键代码：
@Test void testListService() { ... }- 边界场景下出现未处理的异常
- 参数组合逻辑存在未发现的缺陷
- 系统升级时引入回归问题
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。
-
@Lingma-Agent 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@Lingma-Agent 请总结上述讨论并提出解决方案。
-
@Lingma-Agent 请根据讨论内容生成优化代码。
| return serviceInfo.getHosts(); | ||
| } | ||
|
|
There was a problem hiding this comment.
| @@ -172,7 +172,7 @@ | |||
| serviceViews.setTotalCount(page.getTotalCount()); | |||
| serviceViews.setPageNumber(page.getPageNumber()); | |||
| serviceViews.setPagesAvailable(page.getPagesAvailable()); | |||
| for (Service each : services) { | |||
| for (Service each : page.getPageItems()) { | |||
There was a problem hiding this comment.
|
|
||
| private String getRegexString(String target) { | ||
| return StringUtils.isBlank(target) ? Constants.ANY_PATTERN | ||
| : Constants.ANY_PATTERN + target + Constants.ANY_PATTERN; | ||
| } | ||
|
|
There was a problem hiding this comment.
|
|
||
| assertNotNull(result); | ||
| assertEquals(3, result.getTotalCount(), "Total service count should be 3"); | ||
| assertEquals(2, result.getPageNumber(), "Current page number should be 2"); |
There was a problem hiding this comment.
修正分页测试用例的断言逻辑错误
🟢 Minor | 🧹 Code Smells
📋 问题详情
在testListService测试用例（第194行）中，当总记录数为3且分页大小为1时，pagesAvailable应为3而非4。当前断言值会导致测试失败。
💡 解决方案
调整分页计算断言：
-assertEquals(4, result.getPagesAvailable(), "PagesAvailable should = (totalCount / pageSize) + 1");
+assertEquals(3, result.getPagesAvailable(), "PagesAvailable should be ceil(3/1)=3");您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
|
@KomachiSion I have repaired it. |
|
@KomachiSion Is this an unstable test? |
|
|
Please sync develop branch and retry, ut problem has fixed. |
done |
|
|
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
fix #13449
fix service pagination in
CatalogServiceV2Impl.javaand add test.Brief changelog
fix service pagination in
CatalogServiceV2Impl.javaand add revelant test.Verifying this change
Follow this checklist to help us incorporate your contribution quickly and easily:
[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.