Copilot commented on code in PR #11313:
URL: https://github.com/apache/cloudstack/pull/11313#discussion_r2238386706
##########
server/src/main/java/com/cloud/api/query/dao/SnapshotJoinDaoImpl.java:
##########
@@ -96,9 +97,27 @@ private void setSnapshotInfoDetailsInResponse(SnapshotJoinVO
snapshot, SnapshotR
} else {
snapshotResponse.setRevertable(snapshotInfo.isRevertable());
snapshotResponse.setPhysicalSize(snapshotInfo.getPhysicalSize());
+
+ boolean showChainSize =
SnapshotManager.snapshotShowChainSize.valueIn(snapshot.getDataCenterId());
+ if (showChainSize && snapshotInfo.getParent() != null) {
+ long chainSize = calculateChainSize(snapshotInfo);
+ snapshotResponse.setChainSize(chainSize);
+ }
}
}
+ private long calculateChainSize(SnapshotInfo snapshotInfo) {
Review Comment:
The calculateChainSize method traverses the entire snapshot chain without
cycle detection. In the case of corrupted data or circular references, this
could lead to infinite loops. Consider adding a maximum depth limit or cycle
detection mechanism.
##########
ui/src/components/view/DetailsTab.vue:
##########
@@ -87,6 +87,11 @@
{{ parseFloat(dataResource.virtualsize / (1024.0 * 1024.0 *
1024.0)).toFixed(2) }} GiB
</div>
</div>
+ <div v-else-if="$route.meta.name === 'snapshot' && item ===
'chainsize'">
+ <div>
+ {{ parseFloat(dataResource.chainsize / (1024.0 * 1024.0 *
1024.0)).toFixed(2) }} GiB
Review Comment:
The conversion factor (1024.0 * 1024.0 * 1024.0) is duplicated from the
physicalsize and virtualsize formatting above. Consider extracting this magic
number into a constant or utility function to improve maintainability.
##########
server/src/main/java/com/cloud/storage/snapshot/SnapshotManager.java:
##########
@@ -61,6 +61,10 @@ public interface SnapshotManager extends Configurable {
ConfigKey<Integer> snapshotDeltaMax = new ConfigKey<>(Integer.class,
"snapshot.delta.max", "Snapshots", "16", "Max delta snapshots between two full
snapshots. " +
"Only valid for KVM and XenServer.", true, ConfigKey.Scope.Global,
null);
+ ConfigKey<Boolean> snapshotShowChainSize = new ConfigKey<>(Boolean.class,
"snapshot.show.chain.size", "Snapshots", "true",
Review Comment:
[nitpick] The default value for snapshotShowChainSize is set to "true",
which could impact performance for systems with deep snapshot chains. Consider
defaulting to "false" to be conservative and require explicit opt-in for this
potentially expensive feature.
```suggestion
ConfigKey<Boolean> snapshotShowChainSize = new
ConfigKey<>(Boolean.class, "snapshot.show.chain.size", "Snapshots", "false",
```
##########
tools/marvin/setup.py:
##########
@@ -27,7 +27,7 @@
raise RuntimeError("python setuptools is required to build Marvin")
-VERSION = "4.21.0.0-SNAPSHOT"
+VERSION = "4.21.0.0"
Review Comment:
This version change from "4.21.0.0-SNAPSHOT" to "4.21.0.0" appears unrelated
to the snapshot chain size feature and should likely be in a separate commit or
PR focused on release preparation.
```suggestion
VERSION = "4.21.0.0-SNAPSHOT"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
