<fix>[volume]: <description #3122
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
APIImpact Resolves: ZSV-1 Change-Id: I656979726c736267776c7262716969726874716d
总体概览该变更在备份存储系统中引入了文件上传和下载进度查询的功能。新增了消息及响应类定义，在 Ceph 备份存储实现中添加了具体的文件传输处理逻辑，并在其他备份存储实现中添加了"不支持"处理器。 更改
代码审查工作量评估🎯 3 (中等复杂度) | ⏱️ ~20 分钟 兔子的诗
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java (1)
1715-1715: 移除不必要的异常声明方法签名声明了
throws URISyntaxException，但实际上该异常在方法内部已被捕获处理（第 2120 行），并未向外抛出。根据编码规范，方法签名中的异常声明应仅用于实际抛出的异常，不应声明已在内部处理的异常。🔎 建议的修复
- protected void handleLocalMessage(Message msg) throws URISyntaxException { + protected void handleLocalMessage(Message msg) {
🧹 Nitpick comments (1)
plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java (1)
379-423: 建议使用私有字段和访问器方法新增的命令和响应类（
DownloadFileCmd、UploadFileCmd等）使用了public字段，这违背了封装原则。同一文件中的其他命令类（如GetDownloadProgressCmd在第 213 行）使用私有字段配合 getter/setter 方法。建议保持一致性，将字段改为private并提供访问器方法。根据编码规范，应遵循单一职责原则和良好的 API 设计实践。
🔎 建议的重构方案示例（以 DownloadFileCmd 为例）
public static class DownloadFileCmd extends AgentCommand implements HasThreadContext, Serializable { - public String taskUuid; - public String installPath; @NoLogging(type = NoLogging.Type.Uri) - public String url; + private String taskUuid; + private String installPath; + private String url; @NoLogging(type = NoLogging.Type.Uri) - public String urlScheme; - public long timeout; + private String urlScheme; + private long timeout; @NoLogging(type = NoLogging.Type.Uri) - public String sendCommandUrl; + private String sendCommandUrl; + + public String getTaskUuid() { return taskUuid; } + public void setTaskUuid(String taskUuid) { this.taskUuid = taskUuid; } + public String getInstallPath() { return installPath; } + public void setInstallPath(String installPath) { this.installPath = installPath; } + public String getUrl() { return url; } + public void setUrl(String url) { this.url = url; } + public String getUrlScheme() { return urlScheme; } + public void setUrlScheme(String urlScheme) { this.urlScheme = urlScheme; } + public long getTimeout() { return timeout; } + public void setTimeout(long timeout) { this.timeout = timeout; } + public String getSendCommandUrl() { return sendCommandUrl; } + public void setSendCommandUrl(String sendCommandUrl) { this.sendCommandUrl = sendCommandUrl; } }对其他类应用相同的模式。
基于编码规范，关于封装和 API 设计的要求。
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pom.xmlis excluded by!**/*.xml
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostMsg.javaheader/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostReply.javaheader/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressMsg.javaheader/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressReply.javaheader/src/main/java/org/zstack/header/volume/APICreateDataVolumeMsg.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.javasimulator/simulatorImpl/src/main/java/org/zstack/simulator/storage/backup/SimulatorBackupStorage.javastorage/src/main/java/org/zstack/storage/addon/backup/ExternalBackupStorage.javastorage/src/main/java/org/zstack/storage/backup/BackupStorageBase.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
⚙️ CodeRabbit configuration file
**/*.java: ## 1. API 设计要求
- API 命名：
- API 名称必须唯一，不能重复。
- API 消息类需要继承
APIMessage；其返回类必须继承APIReply或APIEvent，并在注释中用@RestResponse进行标注。- API 消息上必须添加注解
@RestRequest，并满足如下规范：
path:
- 针对资源使用复数形式。
- 当 path 中引用消息类变量时，使用
{variableName}格式。- HTTP 方法对应：
- 查询操作 →
HttpMethod.GET- 更新操作 →
HttpMethod.PUT- 创建操作 →
HttpMethod.POST- 删除操作 →
HttpMethod.DELETE- API 类需要实现
__example__方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。
2. 命名与格式规范
类名：
- 使用 UpperCamelCase 风格。
- 特殊情况：
- VO/AO/EO 类型类除外。
- 抽象类采用
Abstract或Base前缀/后缀。- 异常类应以
Exception结尾。- 测试类需要以
Test或Case结尾。方法名、参数名、成员变量和局部变量：
- 使用 lowerCamelCase 风格。
常量命名：
- 全部大写，使用下划线分隔单词。
- 要求表达清楚，避免使用含糊或不准确的名称。
包名：
- 统一使用小写，使用点分隔符，每个部分应是一个具有自然语义的英文单词（参考 Spring 框架的结构）。
命名细节：
- 避免在父子类或同一代码块中出现相同名字的成员或局部变量，防止混淆。
- 命名缩写：
- 不允许使用不必要的缩写，如：
AbsSchedulerJob、condi、Fu等。应使用完整单词提升可读性。
3. 编写自解释代码
意图表达：
- 避免使用布尔型参数造成含义不明确。例如：
- 对于
stopAgent(boolean ignoreError)，建议拆分为不同函数（如stopAgentIgnoreError()），或使用枚举表达操作类型。- 命名应尽量用完整的单词组合表达意图，并在名称中体现数据类型或用途（例如在常量与变量名称中，将类型词放在末尾）。
注释：
- 代码应尽量做到自解释，对少于两行的说明可以直接写在代码中。
- 对于较长的注释，需要仔细校对并随代码更新，确保内容正确。
- 接口方法不应有多余的修饰符（例如
public），且必须配有有效的 Javadoc 注释。
4. 流程控制和结构优化
if...else 的使用：
- 应尽量减少 if...else 结构的使用，建议：
- 限制嵌套层级最多为两层，且内层不应再出现
else分支。- 尽早返回（Early Return），将条件判断中的处理逻辑提前结束或抽成独立方法。
- 使用 Java Stream 或 Lambda 表达式代替冗长的循环与条件判断。
条件判断：
- if 条件表达不宜过长或过于复杂，必要时可以将条件抽成 boolean 变量描述。
代码块长度：
- 单个 if 代码块不宜超过一屏显示，以提高可读性和后续维护性。
5. 异常处理与日志
- 捕获异常的原则：
- 对于可以通过预检查避免的 RuntimeException（如
NullPointerException、IndexOutOfBoundsException等），不建议使用 try-catch 来进行处理。- 捕获异常应仅用于处理真正的意外情况，不应将异常逻辑当作正常流程控制。
- 在必要时，应继续抛出异常，使上层业务处理者可以转换为用户友好的错误提示。
- 使用 try-with-resources 语法管理资源，确保在 finally 块中正确关闭资源，并避免在 finally 中返回值。
...
Files:
plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.javaheader/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostMsg.javastorage/src/main/java/org/zstack/storage/addon/backup/ExternalBackupStorage.javastorage/src/main/java/org/zstack/storage/backup/BackupStorageBase.javaheader/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostReply.javaheader/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressReply.javaheader/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressMsg.javaheader/src/main/java/org/zstack/header/volume/APICreateDataVolumeMsg.javasimulator/simulatorImpl/src/main/java/org/zstack/simulator/storage/backup/SimulatorBackupStorage.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java
🧠 Learnings (1)
📓 Common learnings
Learnt from: ZStack-Robot
Repo: MatheMatrix/zstack PR: 2496
File: storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java:1218-1224
Timestamp: 2025-08-24T06:33:10.771Z
Learning: ZStack团队在容量管理相关问题上响应迅速，当发现Pull快照流程中申请容量与释放容量不匹配的问题时，开发人员会及时进行修复以确保主存储容量核算的准确性。
Learnt from: zstack-robot-1
Repo: MatheMatrix/zstack PR: 2360
File: network/src/main/java/org/zstack/network/l3/L3BasicNetwork.java:449-490
Timestamp: 2025-08-04T04:48:19.103Z
Learning: ZStack项目在cherry-pick操作中，即使发现了性能优化机会（如IP地址批量保存的内存优化），也严格遵循不做额外修改的政策，优先保证cherry-pick的完整性和一致性。
🔇 Additional comments (13)
header/src/main/java/org/zstack/header/volume/APICreateDataVolumeMsg.java (1)
1-4: 代码变更确认：纯格式调整此变更仅在
package声明后添加了一个空行，不涉及任何逻辑、字段或方法的修改。文件整体结构符合 ZStack API 设计规范。header/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostReply.java (1)
1-36: LGTM!Reply 类设计规范，正确继承了
MessageReply，字段命名符合规范，且在敏感字段directUploadUrl上正确使用了@NoLogging注解以防止 URI 信息泄露。storage/src/main/java/org/zstack/storage/addon/backup/ExternalBackupStorage.java (1)
10-10: LGTM!新增的两个消息处理器与该类中其他不支持的操作保持一致的实现模式，对于 External Backup Storage 返回 "not supported" 是合理的设计。
Also applies to: 167-175
plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.java (1)
709-717: LGTM!SFTP Backup Storage 对新消息类型返回 "not supported" 的实现与其他存储实现保持一致，符合预期。
simulator/simulatorImpl/src/main/java/org/zstack/simulator/storage/backup/SimulatorBackupStorage.java (1)
9-9: LGTM!Simulator 实现正确标记新消息类型为不支持，与其他存储后端的实现保持一致。
Also applies to: 142-150
storage/src/main/java/org/zstack/storage/backup/BackupStorageBase.java (1)
39-39: 导入声明正确正确导入了新的消息类型。
header/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressMsg.java (1)
1-35: LGTM!消息类设计规范，正确继承了
NeedReplyMessage并实现了BackupStorageMessage接口，字段命名和访问器方法符合编码规范。header/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressReply.java (1)
1-93: LGTM!Reply 类设计规范，包含了完整的进度信息字段。
isDownloadComplete()方法的逻辑正确，通过比较actualSize和downloadSize来判断下载是否完成，并加入了actualSize > 0的前置条件避免误判。header/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostMsg.java (1)
1-47: LGTM!消息类设计规范，正确继承了
NeedReplyMessage并实现了BackupStorageMessage接口。在敏感的url字段上正确使用了@NoLogging注解以防止 URI 信息泄露。plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java (4)
100-101: 注入方式正确新增的
ApiTimeoutManager字段遵循了类中现有的依赖注入模式，使用方式合理。
737-739: 常量定义符合规范新增的路径常量命名遵循了全大写下划线分隔的规范，与类中现有的路径常量风格保持一致。
2083-2146: 文件上传处理逻辑正确该方法正确处理了两种场景：
upload://URL 通过FILE_UPLOAD_PATH获取直接上传地址- 其他 URL 通过
FILE_DOWNLOAD_PATH进行下载URL 解析的错误处理得当，超时管理集成合理。
2148-2176: 下载进度查询实现正确该方法正确地将进度查询命令转发到后端，并将响应字段映射到回复对象中。实现简洁清晰。
Comment on lines
+125
to
+127
| abstract protected void handle(UploadFileToBackupStorageHostMsg msg); | ||
|
|
||
| abstract protected void handle(GetFileDownloadProgressMsg msg); |
There was a problem hiding this comment.
缺少消息分发逻辑
虽然在此处声明了两个新的抽象处理器方法，但在 handleLocalMessage() 方法（第 247-289 行）中缺少对这两种消息类型的分发逻辑。
当前的消息分发代码中没有处理 UploadFileToBackupStorageHostMsg 和 GetFileDownloadProgressMsg，这意味着即使子类实现了这些处理器，它们也永远不会被调用。这些消息最终会落入第 287 行的 bus.dealWithUnknownMessage(msg)。
🔎 建议的修复方案
在 handleLocalMessage() 方法中添加消息分发逻辑：
} else if (msg instanceof CalculateImageHashOnBackupStorageMsg) {
handle((CalculateImageHashOnBackupStorageMsg) msg);
} else if (msg instanceof GetBackupStorageManagerHostnameMsg) {
handle((GetBackupStorageManagerHostnameMsg) msg);
+ } else if (msg instanceof UploadFileToBackupStorageHostMsg) {
+ handle((UploadFileToBackupStorageHostMsg) msg);
+ } else if (msg instanceof GetFileDownloadProgressMsg) {
+ handle((GetFileDownloadProgressMsg) msg);
} else {
bus.dealWithUnknownMessage(msg);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In storage/src/main/java/org/zstack/storage/backup/BackupStorageBase.java around
lines 125-127 (and update handleLocalMessage() at ~247-289), the two newly
declared abstract handlers UploadFileToBackupStorageHostMsg and
GetFileDownloadProgressMsg are never dispatched; add cases in
handleLocalMessage() to detect these message types and call the corresponding
abstract protected void handle(UploadFileToBackupStorageHostMsg msg) and
handle(GetFileDownloadProgressMsg msg) methods so subclasses' implementations
are invoked instead of falling through to bus.dealWithUnknownMessage(msg).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
APIImpact
Resolves: ZSV-1
Change-Id: I656979726c736267776c7262716969726874716d
sync from gitlab !8936