Copilot commented on code in PR #14259:
URL: https://github.com/apache/cloudstack/pull/14259#discussion_r4218231740
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -1446,11 +1446,12 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd)
throws ResourceAllocationExcep
if (currentSize != newSize ||
!compareEqualsIncludingNullOrZero(newMaxIops, volume.getMaxIops()) ||
!compareEqualsIncludingNullOrZero(newMinIops, volume.getMinIops())) {
volumeResizeRequired = true;
}
- if (!volumeMigrateRequired && !volumeResizeRequired &&
newDiskOffering != null) {
- _volsDao.updateDiskOffering(volume.getId(),
newDiskOffering.getId());
+ if (!volumeMigrateRequired && !volumeResizeRequired) {
volume = _volsDao.findById(volume.getId());
- updateStorageWithTheNewDiskOffering(volume, newDiskOffering);
-
+ if (newDiskOffering != null) {
+ _volsDao.updateDiskOffering(volume.getId(),
newDiskOffering.getId());
+ updateStorageWithTheNewDiskOffering(volume,
newDiskOffering);
+ }
return volume;
}
Review Comment:
`volume` is reloaded *before* updating the disk offering, so the returned
`volume` (and the `volume` passed into `updateStorageWithTheNewDiskOffering`)
can be stale when `newDiskOffering != null`. This differs from the previous
behavior (reload after update) and can cause inconsistent state or incorrect
downstream logic if `updateStorageWithTheNewDiskOffering` expects `volume` to
already reflect the new offering. Consider reloading after
`_volsDao.updateDiskOffering(...)` (or moving the reload into the
`newDiskOffering != null` block and after the update) so both the returned
object and any downstream call sees the updated state.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -1446,11 +1446,12 @@ public VolumeVO resizeVolume(ResizeVolumeCmd cmd)
throws ResourceAllocationExcep
if (currentSize != newSize ||
!compareEqualsIncludingNullOrZero(newMaxIops, volume.getMaxIops()) ||
!compareEqualsIncludingNullOrZero(newMinIops, volume.getMinIops())) {
volumeResizeRequired = true;
}
- if (!volumeMigrateRequired && !volumeResizeRequired &&
newDiskOffering != null) {
- _volsDao.updateDiskOffering(volume.getId(),
newDiskOffering.getId());
+ if (!volumeMigrateRequired && !volumeResizeRequired) {
volume = _volsDao.findById(volume.getId());
Review Comment:
This forces an extra DB fetch even when `newDiskOffering == null` and you’re
returning early because nothing changed. If the intent is only to refresh state
when an update occurred, consider fetching only in the disk-offering update
path (or returning the existing `volume` when no changes were applied) to avoid
unnecessary DAO calls on no-op submissions.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
