DaanHoogland commented on code in PR #7670:
URL: https://github.com/apache/cloudstack/pull/7670#discussion_r1238149173


##########
server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java:
##########
@@ -1156,8 +1156,24 @@ public ResourceCountCheckTask() {
         @Override
         protected void runInContext() {
             s_logger.info("Started resource counters recalculation periodic 
task.");
-            List<DomainVO> domains = 
_domainDao.findImmediateChildrenForParent(Domain.ROOT_DOMAIN);
-            List<AccountVO> accounts = 
_accountDao.findActiveAccountsForDomain(Domain.ROOT_DOMAIN);
+            List<DomainVO> domains;
+            List<AccountVO> accounts;
+            // try/catch task, otherwise it won't be rescheduled in case of 
exception
+            try {
+                domains = 
_domainDao.findImmediateChildrenForParent(Domain.ROOT_DOMAIN);
+            } catch (Exception e) {
+                s_logger.warn("Resource counters recalculation periodic task 
failed, unable to fetch immediate children for the domain " + 
Domain.ROOT_DOMAIN, e);
+                // initialize domains as empty list to do best effort 
recalculation
+                domains = new ArrayList<>();
+            }
+            // try/catch task, otherwise it won't be rescheduled in case of 
exception
+            try {
+                accounts = 
_accountDao.findActiveAccountsForDomain(Domain.ROOT_DOMAIN);
+            } catch (Exception e) {
+                s_logger.warn("Resource counters recalculation periodic task 
failed, unable to fetch active accounts for domain " + Domain.ROOT_DOMAIN, e);
+                // initialize accounts as empty list to do best effort 
recalculation
+                accounts = new ArrayList<>();
+            }

Review Comment:
   does it make sense to keep running if one of these retrievals fail (for this 
pass I mean)?
   I like the logging but i think a little further restructuring makes sense; 
two separate methods, one for domains and one for accounts for instance.
   the recursion at lines 1197 and 1204 looks quite strange, if at all it works 
I would suggest a renaming for readability's sake.



-- 
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]

Reply via email to