Copilot commented on code in PR #12367:
URL: https://github.com/apache/cloudstack/pull/12367#discussion_r2662375480
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -2179,14 +2179,16 @@ public Volume changeDiskOfferingForVolumeInternal(Long
volumeId, Long newDiskOff
}
Collections.shuffle(suitableStoragePoolsWithEnoughSpace);
MigrateVolumeCmd migrateVolumeCmd = new
MigrateVolumeCmd(volume.getId(),
suitableStoragePoolsWithEnoughSpace.get(0).getId(), newDiskOffering.getId(),
true);
+ String volumeUuid = volume.getUuid();
try {
Volume result = migrateVolume(migrateVolumeCmd);
volume = (result != null) ? _volsDao.findById(result.getId())
: null;
if (volume == null) {
- throw new CloudRuntimeException(String.format("Volume
change offering operation failed for volume: %s migration failed to storage
pool %s", volume, suitableStoragePools.get(0)));
+ throw new CloudRuntimeException("Change offering for the
volume failed.");
}
} catch (Exception e) {
- throw new CloudRuntimeException(String.format("Volume change
offering operation failed for volume: %s migration failed to storage pool %s
due to %s", volume, suitableStoragePools.get(0), e.getMessage()));
+ logger.error("Volume change offering operation failed for
volume ID: {} migration failed to storage pool {} due to {}", volumeUuid,
suitableStoragePools.get(0).getId(), e.getMessage());
Review Comment:
The log statement references `suitableStoragePools.get(0).getId()` but
should reference `suitableStoragePoolsWithEnoughSpace.get(0).getId()` to match
the storage pool that was actually used in the MigrateVolumeCmd on line 2181.
This inconsistency could lead to logging the wrong storage pool ID for
debugging purposes.
```suggestion
logger.error("Volume change offering operation failed for
volume ID: {} migration failed to storage pool {} due to {}", volumeUuid,
suitableStoragePoolsWithEnoughSpace.get(0).getId(), e.getMessage());
```
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -2179,14 +2179,16 @@ public Volume changeDiskOfferingForVolumeInternal(Long
volumeId, Long newDiskOff
}
Collections.shuffle(suitableStoragePoolsWithEnoughSpace);
MigrateVolumeCmd migrateVolumeCmd = new
MigrateVolumeCmd(volume.getId(),
suitableStoragePoolsWithEnoughSpace.get(0).getId(), newDiskOffering.getId(),
true);
+ String volumeUuid = volume.getUuid();
try {
Volume result = migrateVolume(migrateVolumeCmd);
volume = (result != null) ? _volsDao.findById(result.getId())
: null;
if (volume == null) {
- throw new CloudRuntimeException(String.format("Volume
change offering operation failed for volume: %s migration failed to storage
pool %s", volume, suitableStoragePools.get(0)));
+ throw new CloudRuntimeException("Change offering for the
volume failed.");
Review Comment:
The error message "Change offering for the volume failed." does not include
the volume ID/UUID, making it difficult for users or administrators to identify
which volume the error relates to. Consider including the volume UUID in the
error message to be consistent with the error message at line 2204 and to help
users identify the affected volume.
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -2199,7 +2201,7 @@ public Volume changeDiskOfferingForVolumeInternal(Long
volumeId, Long newDiskOff
if (volumeMigrateRequired) {
logger.warn(String.format("Volume change offering
operation succeeded for volume ID: %s but volume resize operation failed, so
please try resize volume operation separately", volume.getUuid()));
} else {
- throw new CloudRuntimeException(String.format("Volume
change offering operation failed for volume ID: %s due to resize volume
operation failed", volume.getUuid()));
+ throw new CloudRuntimeException(String.format("Volume disk
offering change operation failed for volume ID [%s] because the volume resize
operation failed.", volume.getUuid()));
Review Comment:
The error message format uses square brackets around the volume ID ("volume
ID [%s]") which is inconsistent with the rest of the codebase. Most other error
messages in this file use "volume ID: %s" format (e.g., lines 1327, 1332, 1356,
2202). Consider changing to maintain consistency: "Volume disk offering change
operation failed for volume ID: %s because the volume resize operation failed."
```suggestion
throw new CloudRuntimeException(String.format("Volume
disk offering change operation failed for volume ID: %s because the volume
resize operation failed.", volume.getUuid()));
```
##########
server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java:
##########
@@ -2179,14 +2179,16 @@ public Volume changeDiskOfferingForVolumeInternal(Long
volumeId, Long newDiskOff
}
Collections.shuffle(suitableStoragePoolsWithEnoughSpace);
MigrateVolumeCmd migrateVolumeCmd = new
MigrateVolumeCmd(volume.getId(),
suitableStoragePoolsWithEnoughSpace.get(0).getId(), newDiskOffering.getId(),
true);
+ String volumeUuid = volume.getUuid();
try {
Volume result = migrateVolume(migrateVolumeCmd);
volume = (result != null) ? _volsDao.findById(result.getId())
: null;
if (volume == null) {
- throw new CloudRuntimeException(String.format("Volume
change offering operation failed for volume: %s migration failed to storage
pool %s", volume, suitableStoragePools.get(0)));
+ throw new CloudRuntimeException("Change offering for the
volume failed.");
}
} catch (Exception e) {
- throw new CloudRuntimeException(String.format("Volume change
offering operation failed for volume: %s migration failed to storage pool %s
due to %s", volume, suitableStoragePools.get(0), e.getMessage()));
+ logger.error("Volume change offering operation failed for
volume ID: {} migration failed to storage pool {} due to {}", volumeUuid,
suitableStoragePools.get(0).getId(), e.getMessage());
+ throw new CloudRuntimeException("Change offering for the
volume failed.", e);
Review Comment:
The error message "Change offering for the volume failed." does not include
the volume ID/UUID, making it difficult for users or administrators to identify
which volume the error relates to. Consider including the volume UUID (which is
already captured in the volumeUuid variable on line 2182) in the error message
to be consistent with the error message at line 2204 and to help users identify
the affected volume.
--
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]
