Copilot commented on code in PR #13410:
URL: https://github.com/apache/cloudstack/pull/13410#discussion_r3528316120
##########
server/src/main/java/com/cloud/capacity/CapacityManagerImpl.java:
##########
@@ -698,14 +706,29 @@ public void updateCapacityForHost(final Host host) {
logger.debug("Found {} VMs are Migrating from {}",
vosMigrating.size(), host);
vms.addAll(vosMigrating);
+
+ List<VMInstanceVO> vmsByLastHostId =
_vmDao.listByLastHostId(host.getId());
+ logger.debug("Found {} VM, not running on {}", vmsByLastHostId.size(),
host);
+
+ List<Long> allVmIds = new ArrayList<>(vms.size() +
vmsByLastHostId.size());
+ for (VMInstanceVO vm : vms) {
+ allVmIds.add(vm.getId());
+ }
+ for (VMInstanceVO vm : vmsByLastHostId) {
+ allVmIds.add(vm.getId());
+ }
+ Map<Long, Map<String, String>> allVmDetails = allVmIds.isEmpty()
+ ? Collections.emptyMap()
+ : batchGetVmDetailsForCapacityCalculation(allVmIds);
+
Review Comment:
`allVmIds` is currently built with *all* VMs returned by `listByLastHostId`,
but VM details are only read for that list when `secondsSinceLastUpdate <
_vmCapacityReleaseInterval`. This means the batch details query can become much
larger than necessary (potentially thousands of stale VMs), which can offset
the N+1 improvement and increase DB load.
##########
engine/schema/src/main/java/org/apache/cloudstack/resourcedetail/ResourceDetailsDaoBase.java:
##########
@@ -122,6 +123,31 @@ public Map<String, String> listDetailsKeyPairs(long
resourceId, List<String> key
return results.stream().collect(Collectors.toMap(R::getName,
R::getValue));
}
+ @Override
+ public Map<Long, Map<String, String>> listDetailsKeyPairs(List<Long>
resourceIds, List<String> keys) {
+ if (CollectionUtils.isEmpty(resourceIds)) {
+ return new HashMap<>();
+ }
+ SearchBuilder<R> sb = createSearchBuilder();
+ sb.and("resourceId", sb.entity().getResourceId(),
SearchCriteria.Op.IN);
+ sb.and("name", sb.entity().getName(), SearchCriteria.Op.IN);
+ sb.done();
+
+ Map<Long, Map<String, String>> result = new
HashMap<>(resourceIds.size());
+ for (int i = 0; i < resourceIds.size(); i += IN_CLAUSE_BATCH_SIZE) {
+ List<Long> batch = resourceIds.subList(i, Math.min(i +
IN_CLAUSE_BATCH_SIZE, resourceIds.size()));
+ SearchCriteria<R> sc = sb.create();
+ sc.setParameters("resourceId", batch.toArray());
+ sc.setParameters("name", keys.toArray());
+ List<R> results = search(sc, null);
+ for (R r : results) {
+ result.computeIfAbsent(r.getResourceId(), k -> new HashMap<>())
+ .put(r.getName(), r.getValue());
+ }
Review Comment:
The new batch implementation silently overwrites duplicate `(resourceId,
name)` pairs via `Map.put(...)`, while the existing single-resource
`listDetailsKeyPairs(long, List<String>)` uses `Collectors.toMap(...)` which
throws on duplicate keys. This makes the batch and non-batch paths behave
differently under the same data corruption scenario; consider failing fast here
too for consistency.
--
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]
