Copilot commented on code in PR #10419:
URL: https://github.com/apache/cloudstack/pull/10419#discussion_r2726858773
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/allocator/AbstractStoragePoolAllocator.java:
##########
@@ -282,22 +270,28 @@ private List<StoragePool>
reorderPoolsByDiskProvisioningType(List<StoragePool> p
reorderedPools.add(preferredIndex++, pool);
}
}
+ logger.debug("Reordered list of pools by disk provisioning type
[{}]: [{}]", diskProfile.getProvisioningType(), pools);
Review Comment:
This debug statement logs the original `pools` list even though the method
has just built a `reorderedPools` list and is about to return it, which makes
the message misleading when troubleshooting reordering by disk provisioning
type. Consider logging the reordered list instead so the log output reflects
the actual ordering that will be used.
```suggestion
logger.debug("Reordered list of pools by disk provisioning type
[{}]: [{}]", diskProfile.getProvisioningType(), reorderedPools);
```
##########
engine/storage/src/main/java/org/apache/cloudstack/storage/allocator/AbstractStoragePoolAllocator.java:
##########
@@ -206,16 +206,11 @@ protected List<StoragePool>
reorderPoolsByNumberOfVolumes(DeploymentPlan plan, L
@Override
public List<StoragePool> reorderPools(List<StoragePool> pools,
VirtualMachineProfile vmProfile, DeploymentPlan plan, DiskProfile dskCh) {
- if (logger.isTraceEnabled()) {
- logger.trace("reordering pools");
- }
if (pools == null) {
- logger.trace("There are no pools to reorder; returning null.");
+ logger.info("There are no pools to reorder.");
return null;
}
- if (logger.isTraceEnabled()) {
- logger.trace(String.format("reordering %d pools", pools.size()));
- }
+ logger.info("Reordering [{}] pools", pools.size());
Review Comment:
These log statements (and similar ones added below) have been raised to INFO
level, which means every pool reordering attempt will now be emitted at INFO
instead of DEBUG/TRACE and can significantly increase log volume in busy
environments. This also goes beyond the PR description (which mentioned adding
DEBUG logs and promoting TRACE to DEBUG), so please confirm that INFO is the
intended level for these high-frequency paths or consider keeping them at DEBUG
while still adopting the new Log4j2 placeholder syntax.
--
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]
