<fix>[storage]: <description#4078
Open
MatheMatrix wants to merge 1 commit into
Open
Conversation
Walkthrough新增快照组树数据模型与文档，提供 VolumeSnapshotGroupTreeBuilder 构建组树并在 VolumeSnapshotManagerImpl 中 marshal 到 APIQueryVolumeSnapshotTreeReply.groupTrees；同时调整快照组解散逻辑以按剩余引用计数解散组。 Changes快照组树查询功能实现
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/tao.gan/4.10.26-ZSV-9792
branch
3 times, most recently
from
3c767f2 to
e1505d8
Compare
![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
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReply.java (1)
25-31: ⚡ Quick win建议在
__example__中补充groupTrees示例数据新增返回字段后，建议同步在示例回复中填充该字段，避免 API 文档示例与实际返回契约脱节。
参考修改
@@ - return reply; + VolumeSnapshotGroupTreeInventory groupInv = new VolumeSnapshotGroupTreeInventory(); + groupInv.setUuid(uuid()); + groupInv.setName("My Snapshot Group"); + groupInv.setVmInstanceUuid(uuid()); + groupInv.setCurrent(true); + groupInv.setIncomplete(false); + groupInv.setSynthetic(false); + groupInv.setChildren(new java.util.ArrayList<>()); + groupInv.setRefs(new java.util.ArrayList<>()); + + reply.setGroupTrees(java.util.Collections.singletonList(groupInv)); + return reply;As per coding guidelines
API 类需要实现 __example__ 方法以便生成 API 文档，并确保生成对应的 Groovy API Template 与 API Markdown 文件。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@header/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReply.java` around lines 25 - 31, Add example data for the new return field groupTrees in the API example generator: update the APIQueryVolumeSnapshotTreeReply.__example__ method to populate groupTrees with one or more VolumeSnapshotGroupTreeInventory instances (using existing helper/example builders for VolumeSnapshotGroupTreeInventory or creating minimal example entries), ensuring the example mirrors the actual returned contract so the generated Groovy API template and Markdown include groupTrees; modify the __example__ implementation inside APIQueryVolumeSnapshotTreeReply to set reply.setGroupTrees(...) accordingly and ensure any nested snapshot inventory example data is included.storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.java (1)
1578-1585: ⚡ Quick win修正：
getChildren()默认已初始化，空指针风险不成立
VolumeSnapshotGroupTreeInventory（header 包）中的children字段默认new ArrayList<>()，getChildren()默认返回非空列表，因此该调用处主要不涉及getChildren()返回 null 导致的 NPE。
- 可选：在
else分支对groupNodeMap.get(node.getParentGroupUuid())为 null 做防御以增强健壮性。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.java` around lines 1578 - 1585, The loop assumes parent entries exist in groupNodeMap before adding children; although getChildren() on VolumeSnapshotGroupTreeInventory is non-null, you should defensively check that groupNodeMap.get(node.getParentGroupUuid()) is not null before calling getChildren() to avoid a potential NPE; update the else branch that references groupNodeMap.get(node.getParentGroupUuid()) (and uses getChildren()) to first retrieve the parent node into a local variable, verify it is non-null (log/warn or skip the child if missing), then add the node to parent.getChildren().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.java`:
- Around line 1613-1629: The class VolumeSnapshotManagerImpl contains duplicate
method definitions for lookupVolumeName and lookupVolumeType which will fail
compilation; remove the repeated copies so each method (lookupVolumeName(String
volumeUuid) and lookupVolumeType(String volumeUuid)) is defined only once, and
if neither is referenced anywhere remove both dead methods entirely; ensure you
leave a single correct implementation (or none) inside VolumeSnapshotManagerImpl
and run a build to verify no remaining references break.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReply.java`:
- Around line 25-31: Add example data for the new return field groupTrees in the
API example generator: update the APIQueryVolumeSnapshotTreeReply.__example__
method to populate groupTrees with one or more VolumeSnapshotGroupTreeInventory
instances (using existing helper/example builders for
VolumeSnapshotGroupTreeInventory or creating minimal example entries), ensuring
the example mirrors the actual returned contract so the generated Groovy API
template and Markdown include groupTrees; modify the __example__ implementation
inside APIQueryVolumeSnapshotTreeReply to set reply.setGroupTrees(...)
accordingly and ensure any nested snapshot inventory example data is included.
In
`@storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.java`:
- Around line 1578-1585: The loop assumes parent entries exist in groupNodeMap
before adding children; although getChildren() on
VolumeSnapshotGroupTreeInventory is non-null, you should defensively check that
groupNodeMap.get(node.getParentGroupUuid()) is not null before calling
getChildren() to avoid a potential NPE; update the else branch that references
groupNodeMap.get(node.getParentGroupUuid()) (and uses getChildren()) to first
retrieve the parent node into a local variable, verify it is non-null (log/warn
or skip the child if missing), then add the node to parent.getChildren().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef188260-7bab-4a96-8e63-4da9d1099b3b
📒 Files selected for processing (10)
header/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReply.javaheader/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeInventory.javaheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeInventoryDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeRefInventory.javaheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeRefInventoryDoc_zh_cn.groovysdk/src/main/java/org/zstack/sdk/QueryVolumeSnapshotTreeResult.javasdk/src/main/java/org/zstack/sdk/VolumeSnapshotGroupTreeInventory.javasdk/src/main/java/org/zstack/sdk/VolumeSnapshotGroupTreeRefInventory.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.java
MatheMatrix
force-pushed
the
sync/tao.gan/4.10.26-ZSV-9792
branch
8 times, most recently
from
a491b89 to
b17f350
Compare
Add groupTrees field to APIQueryVolumeSnapshotTreeReply, built from VolumeSnapshotGroupVO topology with vote-majority parent resolution. Extract build logic into VolumeSnapshotGroupTreeBuilder. Resolves: ZSV-9792 Change-Id: I71616f6f6f6c637a6167637863727575746f616c
MatheMatrix
force-pushed
the
sync/tao.gan/4.10.26-ZSV-9792
branch
from
b17f350 to
7742db4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@storage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupTreeBuilder.java`:
- Around line 216-229: The current winner selection loop in
VolumeSnapshotGroupTreeBuilder uses votes, winner, and tie and is
non-deterministic when vote counts tie; modify the selection so that when
e.getValue() == top you break the tie deterministically by comparing group UUID
strings (e.getKey()) lexicographically (or another stable comparator) and
updating winner to the smaller/consistent key; ensure the loop uses
votes.entrySet() as before but applies the secondary sort on tie, updates
tie=false only when strict tie resolution isn't needed, and keep the existing
logger.warn(String.format(..., selfGroupUuid, votes, winner)) behavior to log
the chosen deterministic winner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c2b48fe4-34de-43ba-8f5b-d8ea8d9e2856
📒 Files selected for processing (12)
header/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReply.javaheader/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeInventory.javaheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeInventoryDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeRefInventory.javaheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeRefInventoryDoc_zh_cn.groovysdk/src/main/java/org/zstack/sdk/QueryVolumeSnapshotTreeResult.javasdk/src/main/java/org/zstack/sdk/VolumeSnapshotGroupTreeInventory.javasdk/src/main/java/org/zstack/sdk/VolumeSnapshotGroupTreeRefInventory.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.javastorage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupTreeBuilder.java
✅ Files skipped from review due to trivial changes (3)
- header/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeRefInventoryDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeInventoryDoc_zh_cn.groovy
- header/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReplyDoc_zh_cn.groovy
Comment on lines
+216
to
+229
| for (Map.Entry<String, Integer> e : votes.entrySet()) { | ||
| if (e.getValue() > top) { | ||
| winner = e.getKey(); | ||
| top = e.getValue(); | ||
| tie = false; | ||
| } else if (e.getValue() == top) { | ||
| tie = true; | ||
| } | ||
| } | ||
| if (tie) { | ||
| logger.warn(String.format("group[uuid:%s] has tied parentGroup votes: %s; picking %s", | ||
| selfGroupUuid, votes, winner)); | ||
| } | ||
| return winner; |
There was a problem hiding this comment.
并列票数时父组选择存在非确定性。
Line 216-224 在 HashMap 上遍历选 winner，票数并列时会受迭代顺序影响，可能导致同一数据集返回不同 parentGroupUuid。建议在并列时增加稳定的二级排序（例如按 groupUuid 字典序）以保证结果可重复。
🔧 建议修复
- String winner = null;
- int top = -1;
- boolean tie = false;
- for (Map.Entry<String, Integer> e : votes.entrySet()) {
- if (e.getValue() > top) {
- winner = e.getKey();
- top = e.getValue();
- tie = false;
- } else if (e.getValue() == top) {
- tie = true;
- }
- }
+ String winner = null;
+ int top = -1;
+ boolean tie = false;
+ List<Map.Entry<String, Integer>> ordered = new ArrayList<>(votes.entrySet());
+ ordered.sort(Comparator.comparing(Map.Entry::getKey));
+ for (Map.Entry<String, Integer> e : ordered) {
+ if (e.getValue() > top) {
+ winner = e.getKey();
+ top = e.getValue();
+ tie = false;
+ } else if (e.getValue() == top) {
+ tie = true;
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@storage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupTreeBuilder.java`
around lines 216 - 229, The current winner selection loop in
VolumeSnapshotGroupTreeBuilder uses votes, winner, and tie and is
non-deterministic when vote counts tie; modify the selection so that when
e.getValue() == top you break the tie deterministically by comparing group UUID
strings (e.getKey()) lexicographically (or another stable comparator) and
updating winner to the smaller/consistent key; ensure the loop uses
votes.entrySet() as before but applies the secondary sort on tie, updates
tie=false only when strict tie resolution isn't needed, and keep the existing
logger.warn(String.format(..., selfGroupUuid, votes, winner)) behavior to log
the chosen deterministic winner.
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.
Resolves: ZSV-9792
Change-Id: I71616f6f6f6c637a6167637863727575746f616c
sync from gitlab !9977