[GitHub] DaanHoogland commented on a change in pull request #2739: Async jobs add endtime

[GitBox <gi...@apache.org>]

DaanHoogland commented on a change in pull request #2739: Async jobs add endtime
URL: https://github.com/apache/cloudstack/pull/2739#discussion_r202337269
 
 

 ##########
 File path: framework/jobs/src/main/java/org/apache/cloudstack/framework/jobs/impl/AsyncJobManagerImpl.java
 ##########
 @@ -1049,8 +1054,8 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
                             }
                         }
                     }
-                    List<SnapshotDetailsVO> snapshotList = _snapshotDetailsDao.findDetails(AsyncJob.Constants.MS_ID, Long.toString(msid), false);
-                    for (SnapshotDetailsVO snapshotDetailsVO : snapshotList) {
+                    final List<SnapshotDetailsVO> snapshotList = _snapshotDetailsDao.findDetails(AsyncJob.Constants.MS_ID, Long.toString(msid), false);
 
 Review comment:
   Actually it is kind of kidding yourself in this case, as the object ref may not be changed after setting but the state can still be changed. I would advice using a final filter against this as a spam filter against spam; ignore, don't read.
   In this case it is best to follow @rafaelweingartner 's suggestion, to make the new and/or changed code be in smaller methods and leave the existing alone. Though refactoring the whole method is an option as well of course ;)

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services