<feature>[longjob]: standardize LongJob progress detail format#3450
Open
zstack-robot-1 wants to merge 2 commits into5.5.12from
Open
<feature>[longjob]: standardize LongJob progress detail format#3450zstack-robot-1 wants to merge 2 commits into5.5.12from
zstack-robot-1 wants to merge 2 commits into5.5.12from
Conversation
Add LongJobProgressDetail POJO and LongJobProgressDetailBuilder to normalize opaque progress data into unified typed structure. Three opaque formats parsed: VM migration, AI download, unknown. DB schema unchanged. Agent-side migration deferred to Phase 2. Resolves: ZSTAC-82318 Change-Id: I70d60ff5e6c8f659f55770e2fbbe56781b238fd5
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml) Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough本次变更新增并传播了标准化的长任务进度详情：添加 Changes
Sequence Diagram(s)sequenceDiagram
participant Service as ProgressReportService
participant Builder as LongJobProgressDetailBuilder
participant Inventory as TaskProgressInventory
participant Notifier as LongJobProgressNotificationMessage
participant API as APIGetTaskProgressReply
Service->>Builder: fromTaskProgressVO(vo)
Builder-->>Service: LongJobProgressDetail
Service->>Inventory: inventory.setProgressDetail(detail)
Service->>Notifier: include progressDetail in notification
Notifier->>API: reply/example includes progressDetail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/longjob/LongJobProgressNotificationMessage.java (1)
100-102: 考虑数据一致性风险
progressDetail同时存储在TaskProgressInventory和LongJobProgressNotificationMessage两处。当前实现中两者指向同一个对象引用，但如果后续代码单独修改其中一个，可能导致数据不一致。建议考虑以下方案之一：
- 仅在
taskProgress中保留progressDetail，移除 message 级别的冗余字段- 在 Javadoc 中明确说明
msg.progressDetail是taskProgress.progressDetail的便捷访问器🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/longjob/LongJobProgressNotificationMessage.java` around lines 100 - 102, The message stores progressDetail redundantly which risks inconsistency; remove the redundant LongJobProgressNotificationMessage.progressDetail field and stop assigning it (replace the lines setting msg.progressDetail with relying on msg.taskProgress.getProgressDetail()), update usages to read progress via taskProgress.getProgressDetail(), and add a Javadoc on LongJobProgressNotificationMessage (or TaskProgressInventory accessor) explaining progressDetail should be accessed through taskProgress to make the invariant clear.header/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.java (1)
38-43: 日志中避免记录完整的 opaque 数据trace 日志中直接拼接
vo.getOpaque()可能导致：
- 日志过长（opaque 可能包含大量数据）
- 潜在的敏感信息泄露
建议截断或摘要显示：
♻️ 建议的修改
} catch (Exception e) { - logger.trace("LongJobProgressDetailBuilder: opaque is not a JSON object, skipping: " + vo.getOpaque(), e); + logger.trace("LongJobProgressDetailBuilder: opaque is not a JSON object, skipping (length={})", + vo.getOpaque() != null ? vo.getOpaque().length() : 0, e); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@header/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.java` around lines 38 - 43, The trace log in LongJobProgressDetailBuilder currently concatenates vo.getOpaque() which can be huge or contain sensitive data; change the logger.trace call in the catch block that handles JSONObjectUtil.toObject(...) to avoid logging the full opaque: log a truncated snippet (e.g. first N chars) or a digest (hash) plus the opaque length and context instead of vo.getOpaque(), and keep the original exception parameter; update the message produced by logger.trace(...) accordingly so it references vo.getUuid() or other non-sensitive identifier along with the truncated/hashed opaque preview.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.java`:
- Around line 38-43: The trace log in LongJobProgressDetailBuilder currently
concatenates vo.getOpaque() which can be huge or contain sensitive data; change
the logger.trace call in the catch block that handles
JSONObjectUtil.toObject(...) to avoid logging the full opaque: log a truncated
snippet (e.g. first N chars) or a digest (hash) plus the opaque length and
context instead of vo.getOpaque(), and keep the original exception parameter;
update the message produced by logger.trace(...) accordingly so it references
vo.getUuid() or other non-sensitive identifier along with the truncated/hashed
opaque preview.
In
`@header/src/main/java/org/zstack/header/longjob/LongJobProgressNotificationMessage.java`:
- Around line 100-102: The message stores progressDetail redundantly which risks
inconsistency; remove the redundant
LongJobProgressNotificationMessage.progressDetail field and stop assigning it
(replace the lines setting msg.progressDetail with relying on
msg.taskProgress.getProgressDetail()), update usages to read progress via
taskProgress.getProgressDetail(), and add a Javadoc on
LongJobProgressNotificationMessage (or TaskProgressInventory accessor)
explaining progressDetail should be accessed through taskProgress to make the
invariant clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 5509151d-a09d-4893-967a-bfd468be5045
⛔ Files ignored due to path filters (2)
sdk/src/main/java/org/zstack/sdk/LongJobProgressDetail.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/TaskProgressInventory.javais excluded by!sdk/**
📒 Files selected for processing (6)
core/src/main/java/org/zstack/core/progress/ProgressReportService.javaheader/src/main/java/org/zstack/header/core/progress/APIGetTaskProgressReply.javaheader/src/main/java/org/zstack/header/core/progress/LongJobProgressDetail.javaheader/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.javaheader/src/main/java/org/zstack/header/core/progress/TaskProgressInventory.javaheader/src/main/java/org/zstack/header/longjob/LongJobProgressNotificationMessage.java
MatheMatrix
force-pushed
the
sync/ye.zou/fix/ZSTAC-82318
branch
from
241090b to
c0bfab9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@header/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.java`:
- Around line 198-210: LongJobProgressDetailBuilder currently only copies
unmapped keys from the progress map into detail.setExtra, which omits unknown
top-level fields in the inner object and non-"data" keys from the raw outer
object; update the builder to merge and preserve all unknown fields by
collecting leftover keys from progress, any unknown keys present in the inner
map, and any keys from the raw map except "data" into a single extra map before
calling detail.setExtra (do this for both the progress->extra block and the
similar block handling lines 213-221), ensuring existing mapped keys (percent,
downloaded_bytes, etc.) are excluded so we don't overwrite mapped fields.
- Around line 157-160: The current code in LongJobProgressDetailBuilder takes
the reported Number percent, rounds it and assigns it directly via
detail.setPercent, which can pass through out-of-range values; modify the logic
that handles the local variable percent (from progress.get("percent")) so after
computing int p = (int) Math.round(percent.doubleValue()) you clamp p to the
0..100 range (e.g., p = Math.max(0, Math.min(100, p))) before calling
detail.setPercent(p) to prevent negative or >100 values reaching the front end.
In
`@header/src/main/java/org/zstack/header/core/progress/TaskProgressInventory.java`:
- Around line 21-22: TaskProgressInventory's field comment for progressDetail
(type LongJobProgressDetail) is inaccurate: "unrecognized" currently does not
yield null but returns a detail with extra; update the Javadoc for the private
LongJobProgressDetail progressDetail to state that it is null only when opaque
is absent or when parsing of opaque fails, and that unrecognized opaque values
produce a detail containing extra information instead of null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: 2d84d1b8-e63a-4910-8340-e6224e126920
⛔ Files ignored due to path filters (3)
sdk/src/main/java/SourceClassMap.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/LongJobProgressDetail.javais excluded by!sdk/**sdk/src/main/java/org/zstack/sdk/TaskProgressInventory.javais excluded by!sdk/**
📒 Files selected for processing (6)
core/src/main/java/org/zstack/core/progress/ProgressReportService.javaheader/src/main/java/org/zstack/header/core/progress/APIGetTaskProgressReply.javaheader/src/main/java/org/zstack/header/core/progress/LongJobProgressDetail.javaheader/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.javaheader/src/main/java/org/zstack/header/core/progress/TaskProgressInventory.javaheader/src/main/java/org/zstack/header/longjob/LongJobProgressNotificationMessage.java
🚧 Files skipped from review as they are similar to previous changes (2)
- header/src/main/java/org/zstack/header/core/progress/APIGetTaskProgressReply.java
- header/src/main/java/org/zstack/header/core/progress/LongJobProgressDetail.java
header/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.java
Show resolved
Hide resolved
header/src/main/java/org/zstack/header/core/progress/LongJobProgressDetailBuilder.java
Outdated
Show resolved
Hide resolved
Comment on lines
+21
to
+22
| /** Typed progress detail parsed from opaque. Null when opaque is absent or unrecognized. */ | ||
| private LongJobProgressDetail progressDetail; |
There was a problem hiding this comment.
字段注释与实际行为不一致，建议修正文案
当前实现下，“unrecognized” 并不会返回 null，而是返回带 extra 的 detail；注释建议改为仅在 opaque 缺失或解析失败时为 null。
建议修复
- /** Typed progress detail parsed from opaque. Null when opaque is absent or unrecognized. */
+ /** Typed progress detail parsed from opaque. Null when opaque is absent or parsing fails. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@header/src/main/java/org/zstack/header/core/progress/TaskProgressInventory.java`
around lines 21 - 22, TaskProgressInventory's field comment for progressDetail
(type LongJobProgressDetail) is inaccurate: "unrecognized" currently does not
yield null but returns a detail with extra; update the Javadoc for the private
LongJobProgressDetail progressDetail to state that it is null only when opaque
is absent or when parsing of opaque fails, and that unrecognized opaque values
produce a detail containing extra information instead of null.
Resolves: ZSTAC-82318 Change-Id: I8931c4207547b836b522c9b5cea2db807a032d5c
MatheMatrix
force-pushed
the
sync/ye.zou/fix/ZSTAC-82318
branch
from
c0bfab9 to
cb554df
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Root Cause
TaskProgressVO.opaque 是自由格式 String，各 agent 的进度详情格式各异：
{remain, total, speed, remaining_migration_time}{data: JSON({state, progress:{...}, state_reason})}(双层嵌套)UI 无法用统一逻辑解析，导致进度展示困难。
Solution
新增
LongJobProgressDetail标准化 POJO +LongJobProgressDetailBuilder适配层：ProgressReportService.inventory()和LongJobProgressNotificationMessage.progressUpdated()中自动将旧格式 opaque 转换为统一结构TaskProgressInventory.progressDetail新字段返回格式适配
Testing
未完成 (Phase 2, 后续迭代)
Resolves: ZSTAC-82318
sync from gitlab !9308