<fix>[volume]: <description #3123
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备份存储中实现完整的处理逻辑，在其他备份存储实现中添加"不支持"响应，并在基类中定义抽象方法作为扩展点。 变更内容
序列图sequenceDiagram
participant Client
participant BackupStorageBase
participant CephBackupStorageBase
participant Agent
participant FileSystem
rect rgb(220, 230, 240)
note over Client,FileSystem: 文件上传流程
Client->>BackupStorageBase: UploadFileToBackupStorageHostMsg
BackupStorageBase->>CephBackupStorageBase: handle(msg)
CephBackupStorageBase->>CephBackupStorageBase: 解析URL、设置超时
CephBackupStorageBase->>Agent: UploadFileCmd
Agent->>FileSystem: 写入文件
FileSystem-->>Agent: 完成，返回md5sum/size
Agent-->>CephBackupStorageBase: UploadFileResponse
CephBackupStorageBase->>Client: UploadFileToBackupStorageHostReply
end
rect rgb(230, 240, 220)
note over Client,FileSystem: 下载进度查询流程
Client->>BackupStorageBase: GetFileDownloadProgressMsg
BackupStorageBase->>CephBackupStorageBase: handle(msg)
CephBackupStorageBase->>Agent: GetDownloadFileProgressCmd
Agent->>FileSystem: 查询下载状态
FileSystem-->>Agent: 返回进度信息
Agent-->>CephBackupStorageBase: GetDownloadFileProgressResponse
CephBackupStorageBase->>Client: GetFileDownloadProgressReply
end
代码审查工作量估算🎯 4 (复杂) | ⏱️ ~45 分钟 原因：涉及7个文件、新的消息体系结构、Ceph实现中复杂的HTTP调用和超时管理逻辑、多个备份存储实现的一致性处理，需要理解消息流、代理命令执行和错误处理机制。 兔子之诗
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.java (1)
692-696: 潜在缺陷：reply 未发送
handle(CalculateImageHashOnBackupStorageMsg)方法创建了reply并设置了错误，但未通过bus.reply(msg, reply)发送。这会导致调用方永远收不到响应，可能造成超时。🔎 建议修复
@Override protected void handle(CalculateImageHashOnBackupStorageMsg msg) { CalculateImageHashOnBackupStorageReply reply = new CalculateImageHashOnBackupStorageReply(); reply.setError(operr("sftp backup storage do not support calculate image hash")); + bus.reply(msg, reply); }
🧹 Nitpick comments (3)
header/src/main/java/org/zstack/header/volume/APICreateDataVolumeMsg.java (1)
3-3: 多余的空行此处新增的空行看起来是无意义的格式变更。如果这不是代码格式化工具的要求，建议移除以保持代码整洁。
simulator/simulatorImpl/src/main/java/org/zstack/simulator/storage/backup/SimulatorBackupStorage.java (1)
141-150: 实现可行，但与部分现有处理器风格不一致新增的处理方法使用
bus.replyErrorByMessageType()返回错误，这是更安全的方式。但请注意，该类中部分现有处理器（如handle(SyncImageSizeOnBackupStorageMsg)在 Lines 100-102）使用的是throw new CloudRuntimeException("not supported yet")。建议保持一致性，或者在后续统一将抛异常的方式改为消息回复方式。
plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java (1)
379-423: 新增命令类使用 public 字段，与现有类风格不一致。当前文件中其他命令类（如
GetDownloadProgressCmd、DownloadCmd等）使用 private 字段配合 getter/setter 方法，而新增的DownloadFileCmd、UploadFileCmd、GetDownloadFileProgressCmd及其响应类直接使用 public 字段。建议保持代码风格一致性。🔎 建议修改示例（以 DownloadFileCmd 为例）
public static class DownloadFileCmd extends AgentCommand implements HasThreadContext, Serializable { - public String taskUuid; - public String installPath; + private String taskUuid; + private String installPath; @NoLogging(type = NoLogging.Type.Uri) - public String url; + 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; + + // Add getters and setters... }
📜 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:
header/src/main/java/org/zstack/header/volume/APICreateDataVolumeMsg.javastorage/src/main/java/org/zstack/storage/addon/backup/ExternalBackupStorage.javaheader/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostReply.javasimulator/simulatorImpl/src/main/java/org/zstack/simulator/storage/backup/SimulatorBackupStorage.javaplugin/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/backup/BackupStorageBase.javaheader/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressMsg.javaheader/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressReply.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java
🧬 Code graph analysis (3)
storage/src/main/java/org/zstack/storage/addon/backup/ExternalBackupStorage.java (1)
header/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostMsg.java (1)
UploadFileToBackupStorageHostMsg(7-46)
simulator/simulatorImpl/src/main/java/org/zstack/simulator/storage/backup/SimulatorBackupStorage.java (1)
header/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostMsg.java (1)
UploadFileToBackupStorageHostMsg(7-46)
storage/src/main/java/org/zstack/storage/backup/BackupStorageBase.java (1)
header/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostMsg.java (1)
UploadFileToBackupStorageHostMsg(7-46)
🔇 Additional comments (9)
storage/src/main/java/org/zstack/storage/addon/backup/ExternalBackupStorage.java (1)
166-175: 实现符合既有模式新增的两个处理方法与该类中其他
handle方法的实现模式一致，均返回 "not supported" 错误。这符合 ExternalBackupStorage 作为外部备份存储桥接层的定位。plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorage.java (1)
708-717: 新增处理方法实现正确新增的两个处理方法正确实现了抽象基类要求，返回 "not supported" 错误。
header/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostReply.java (1)
1-35: 新增回复类结构良好
UploadFileToBackupStorageHostReply类设计合理：
- 正确继承
MessageReply@NoLogging(type = NoLogging.Type.Uri)注解正确应用于directUploadUrl，防止敏感 URL 被记录到日志- 字段命名清晰，getter/setter 符合规范
header/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressMsg.java (1)
1-34: 消息类结构正确
GetFileDownloadProgressMsg类设计符合框架规范：
- 正确继承
NeedReplyMessage并实现BackupStorageMessage接口getBackupStorageUuid()方法正确覆盖接口要求- 字段设计合理，用于查询下载进度
header/src/main/java/org/zstack/header/image/UploadFileToBackupStorageHostMsg.java (1)
1-46: 消息类设计良好
UploadFileToBackupStorageHostMsg类设计符合框架规范：
- 正确继承
NeedReplyMessage并实现BackupStorageMessage接口@NoLogging(type = NoLogging.Type.Uri)注解正确应用于url字段，防止敏感信息泄露- 字段设计完整，包含上传所需的任务标识、URL 和安装路径
plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java (2)
100-101: LGTM!新增的
ApiTimeoutManager注入和端点常量遵循现有代码规范。Also applies to: 737-739
2083-2146: LGTM!该方法正确处理了
upload://协议和其他 URL 的不同逻辑，URI 解析错误处理完善，使用了 Early Return 模式提高可读性。header/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressReply.java (2)
49-51:isDownloadComplete()方法逻辑可能存在语义问题。当前实现
actualSize > 0 && actualSize == downloadSize，但字段命名可能引起混淆：
actualSize通常表示文件的实际大小downloadSize通常表示已下载的大小如果语义如此，判断下载完成的条件应该是
downloadSize == size或downloadSize >= actualSize。请确认字段语义是否正确，或考虑添加注释说明字段含义。
17-91: LGTM!Getter/setter 方法遵循 Java Bean 规范，实现正确。
Comment on lines
+5
to
+15
| public class GetFileDownloadProgressReply extends MessageReply { | ||
| private boolean completed; | ||
| private int progress; | ||
|
|
||
| private long size; | ||
| private long actualSize; | ||
| private long downloadSize; | ||
| private String installPath; | ||
| private String format; | ||
| private long lastOpTime; | ||
| private boolean supportSuspend; |
There was a problem hiding this comment.
缺少 md5sum 字段。
在 CephBackupStorageBase.java 第 2171 行调用了 reply.setMd5sum(rsp.md5sum)，但此类未定义 md5sum 字段，会导致编译错误。
🔎 建议添加 md5sum 字段
public class GetFileDownloadProgressReply extends MessageReply {
private boolean completed;
private int progress;
private long size;
private long actualSize;
private long downloadSize;
private String installPath;
private String format;
private long lastOpTime;
private boolean supportSuspend;
+ private String md5sum;
+
+ public String getMd5sum() {
+ return md5sum;
+ }
+
+ public void setMd5sum(String md5sum) {
+ this.md5sum = md5sum;
+ }🤖 Prompt for AI Agents
In
header/src/main/java/org/zstack/header/storage/backup/GetFileDownloadProgressReply.java
around lines 5 to 15, the class is missing the md5sum field referenced by
CephBackupStorageBase at line 2171; add a private String md5sum field and
provide its public getter and setter (getMd5sum and setMd5sum) so the
reply.setMd5sum(...) call compiles correctly and follows existing Java bean
style used in this class.
Comment on lines
+2148
to
+2176
| @Override | ||
| protected void handle(GetFileDownloadProgressMsg msg) { | ||
| GetFileDownloadProgressReply reply = new GetFileDownloadProgressReply(); | ||
|
|
||
| GetDownloadFileProgressCmd cmd = new GetDownloadFileProgressCmd(); | ||
| cmd.taskUuid = msg.getTaskUuid(); | ||
|
|
||
| httpCall(FILE_UPLOAD_PROGRESS_PATH, cmd, GetDownloadFileProgressResponse.class, new ReturnValueCompletion<GetDownloadFileProgressResponse>(msg) { | ||
| @Override | ||
| public void fail(ErrorCode err) { | ||
| reply.setError(err); | ||
| bus.reply(msg, reply); | ||
| } | ||
|
|
||
| @Override | ||
| public void success(GetDownloadFileProgressResponse rsp) { | ||
| reply.setCompleted(rsp.completed); | ||
| reply.setProgress(rsp.progress); | ||
| reply.setActualSize(rsp.actualSize); | ||
| reply.setSize(rsp.size); | ||
| reply.setInstallPath(rsp.installPath); | ||
| reply.setDownloadSize(rsp.downloadSize); | ||
| reply.setLastOpTime(rsp.lastOpTime); | ||
| reply.setMd5sum(rsp.md5sum); | ||
| reply.setSupportSuspend(rsp.supportSuspend); | ||
| bus.reply(msg, reply); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
编译错误：GetFileDownloadProgressReply 缺少 md5sum 字段。
第 2171 行调用 reply.setMd5sum(rsp.md5sum)，但 GetFileDownloadProgressReply 类中未定义 md5sum 字段和对应的 setter 方法，这将导致编译失败。
需要在 GetFileDownloadProgressReply 类中添加 md5sum 字段。
🤖 Prompt for AI Agents
In
plugin/ceph/src/main/java/org/zstack/storage/ceph/backup/CephBackupStorageBase.java
around lines 2148 to 2176, the code calls reply.setMd5sum(rsp.md5sum) but
GetFileDownloadProgressReply lacks an md5sum field and setter; modify the
GetFileDownloadProgressReply class to add a private String md5sum field plus
public void setMd5sum(String md5sum) and public String getMd5sum() (or matching
types used by rsp.md5sum), then recompile to ensure the reference is resolved.
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.
缺少消息路由处理
新增了抽象方法 handle(UploadFileToBackupStorageHostMsg msg) 和 handle(GetFileDownloadProgressMsg msg)，但在 handleLocalMessage 方法（Lines 247-289）中未添加对应的消息路由分支。
这将导致这两种消息无法被正确分发到 handle 方法，而是进入 bus.dealWithUnknownMessage(msg) 分支。
🔎 建议在 handleLocalMessage 中添加路由
} 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 (new abstract methods) and handleLocalMessage around lines
247-289, the two new abstract handlers UploadFileToBackupStorageHostMsg and
GetFileDownloadProgressMsg are not routed and will hit
bus.dealWithUnknownMessage(msg). Add branches in handleLocalMessage that check
for those message types (instanceof UploadFileToBackupStorageHostMsg and
instanceof GetFileDownloadProgressMsg), invoke the corresponding abstract
handle(...) methods, and return after handling so they do not fall through to
the unknown-message branch; also ensure the message classes are imported and any
reply or exception handling follows the existing pattern used for other message
branches.
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 !8937