[GitHub] rafaelweingartner commented on a change in pull request #2398: CLOUDSTACK-10222: Clean previous snaphosts from primary storage when ?

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

rafaelweingartner commented on a change in pull request #2398: CLOUDSTACK-10222: Clean previous snaphosts from primary storage when ?
URL: https://github.com/apache/cloudstack/pull/2398#discussion_r160486246
 
 

 ##########
 File path: plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/Xenserver625StorageProcessor.java
 ##########
 @@ -576,13 +578,20 @@ public Answer backupSnapshot(final CopyCommand cmd) {
             s_logger.info("New snapshot details: " + newSnapshot.toString());
             s_logger.info("New snapshot physical utilization: "+physicalSize);
 
+            successful = true;
+
             return new CopyCmdAnswer(newSnapshot);
         } catch (final Types.XenAPIException e) {
             details = "BackupSnapshot Failed due to " + e.toString();
             s_logger.warn(details, e);
         } catch (final Exception e) {
             details = "BackupSnapshot Failed due to " + e.getMessage();
             s_logger.warn(details, e);
+        } finally {
+            if (!successful) {
 
 Review comment:
   What about removing this finally and boolean control variable? You only need to worry about exception cases (normal workflow is already addressed at 567-568). So, you can do something like this in the catch block: 
   ```
   destroySnapshotOnPrimaryStorage(conn, snapshotUuid);
   ```
   
   BTW: the method `destroySnapshotOnPrimaryStorage(Conn, String)` does seem to need that boolean return and we also could remove its catch of `Exception`.
     

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