[ISSUE #13514] Fix ConfigMigrateService Bug #13515
Conversation
|
Thanks for your this PR. 🙏 感谢您提交的PR。 🙏 |
修复配置迁移服务中时间戳和MD5校验的逻辑问题变更概述问题修复
数据库变更
逻辑增强
变更统计
变更文件
💡 小贴士与 lingma-agents 交流的方式📜 直接回复评论
📜 在代码行处标记
📜 在讨论中提问
|
There was a problem hiding this comment.
🔎 代码评审报告
🎯 评审意见概览
| 严重度 | 数量 | 说明 |
|---|---|---|
| 🔴 Blocker | 0 | 阻断性问题，需立即修复。例如：系统崩溃、关键功能不可用或严重安全漏洞。 |
| 🟠 Critical | 1 | 严重问题，高优先级修复。例如：核心功能异常或性能瓶颈影响用户体验。 |
| 🟡 Major | 0 | 主要问题，建议修复。例如：非核心功能缺陷或代码维护性较差。 |
| 🟢 Minor | 0 | 次要问题，酬情优化。例如：代码格式不规范或注释缺失。 |
总计: 1 个问题
📋 评审意见详情
💡 代码实现建议
以下是文件级别的代码建议，聚焦于代码的可读性、可维护性和潜在问题。
☕ config/src/main/java/com/alibaba/nacos/config/server/service/repository/embedded/EmbeddedConfigMigratePersistServiceImpl.java (1 💬)
- SQL参数数量与占位符不匹配 (L281)
🚀 架构设计建议
以下是对代码架构和设计的综合分析，聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍1. 条件判断逻辑不一致可能导致数据竞争和覆盖问题
在多个方法中，关于时间戳(lastModified)和MD5的条件判断逻辑存在不一致。例如：
ConfigMigrateService.checkChangedConfigGrayMigrateState中，338行使用>=比较lastModified，而364行使用>=但后续条件组合不同。checkDeletedConfigGrayMigrateState中469行将条件从<改为<=，可能导致数据删除策略变化。ExternalConfigMigratePersistServiceImpl.syncConfig中371行新增>=分支，但未统一其他类似场景的条件。
这种不一致可能导致不同分支下相同逻辑场景的处理结果不一致，引发数据覆盖或删除冲突。建议统一时间戳比较逻辑，并明确各条件组合的业务含义。
📌 关键代码
-
config/src/main/java/com/alibaba/nacos/config/server/service/ConfigMigrateService.java(338-364) -
config/src/main/java/com/alibaba/nacos/config/server/service/ConfigMigrateService.java(469-470)
数据竞争导致配置信息意外覆盖或删除，迁移过程中数据不一致，影响系统稳定性。
🔍2. SQL参数与占位符不匹配问题可能未完全修复
虽然EmbeddedConfigMigratePersistServiceImpl的281行已指出参数不匹配，但其他SQL语句可能存在类似隐患。例如：
updateConfigInfoAtomic方法(274行)新增的md5条件列未在参数列表中正确对应。updateConfigInfo4GrayWithoutHistory(361行)的SQL条件包含md5，但参数传递顺序可能未同步调整。
需要系统性检查所有SQL构造处的参数与占位符数量及顺序是否一致。
📌 关键代码
执行SQL时出现参数绑定错误，导致更新失败或数据污染。
🔍3. 跨存储实现的同步逻辑存在代码重复
Embedded和External存储实现中的syncConfig和syncConfigGray方法存在大量重复逻辑，例如：
- 时间戳比较(
lastModified)和MD5验证的分支判断。 - 租户设置(
setTenant)和数据库更新操作的流程。
未将公共逻辑抽象为基类或工具方法，导致代码冗余，后续维护困难。
📌 关键代码
重复代码导致后续功能修改需同步更新多个地方，增加维护成本并引入不一致性风险。
🔍4. 新增复杂条件分支缺乏测试覆盖
新增的多个条件分支（如370行的else if(lastModified < ...）和参数组合（如targetMd5）未见对应的单元测试。特别是：
- 时间戳相等(
==)场景的处理。 targetMd5与源MD5不同时的更新逻辑。
可能存在未被测试的路径，导致线上环境出现未预期的行为。
📌 关键代码
新增逻辑未经过充分验证，存在潜在缺陷未被发现的风险。
审查详情
📒 文件清单 （3 个文件）
📝 变更: 3 个文件
📝 变更文件:
config/src/main/java/com/alibaba/nacos/config/server/service/ConfigMigrateService.javaconfig/src/main/java/com/alibaba/nacos/config/server/service/repository/embedded/EmbeddedConfigMigratePersistServiceImpl.javaconfig/src/main/java/com/alibaba/nacos/config/server/service/repository/extrnal/ExternalConfigMigratePersistServiceImpl.java
💡 小贴士
与 lingma-agents 交流的方式
📜 直接回复评论
直接回复本条评论，lingma-agents 将自动处理您的请求。例如：
-
在当前代码中添加详细的注释说明。
-
请详细介绍一下你说的 LRU 改造方案，并使用伪代码加以说明。
📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如：
-
@lingma-agents 分析这个方法的性能瓶颈并提供优化建议。
-
@lingma-agents 对这个方法生成优化代码。
📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如：
-
@lingma-agents 请总结上述讨论并提出解决方案。
-
@lingma-agents 请根据讨论内容生成优化代码。
|
|
||
| final Object[] args = new Object[] {configInfo.getContent(), md5Tmp, srcIp, srcUser, appNameTmp, desc, use, | ||
| effect, type, schema, encryptedDataKey, configInfo.getDataId(), configInfo.getGroup(), tenantTmp, | ||
| new Timestamp(lastModified)}; | ||
| new Timestamp(lastModified), targetMd5}; |
There was a problem hiding this comment.
SQL参数数量与占位符不匹配
🟠 Critical | 🐞 Bugs
📋 问题详情
在updateConfigInfoAtomic方法中，新增的targetMd5参数未被正确使用在SQL条件中。当前SQL的where条件仅包含data_id, group_id, tenant_id, gmt_modified, md5字段，但参数数组传递了额外的targetMd5值可能导致参数数量不匹配引发异常。
💡 解决方案
调整参数顺序并移除多余参数：
-new Timestamp(lastModified), targetMd5
+new Timestamp(lastModified)您的反馈对我们很重要！(建议右键在新标签页中打开以下链接)
|
|
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
Fix #13514
Brief changelog
During horizontal notification configuration migration, an Md5 value was added as a judgment condition, and migration operations are also performed for configurations with the same timestamp.
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.