Allon Mureinik has posted comments on this change.

Change subject: core: Consolidate vm down check when removing disk
......................................................................


Patch Set 1: (2 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveDiskCommand.java
Line 79: 
Line 80:         buildSharedLockMap();
Line 81: 
Line 82:         return acquireLockInternal() &&
Line 83:                 validateAllVmsForDiskAreDown() &&
in psuedo-code, what happened here is the following:

old code:
if diskStorageType == image
  someValidations()
  if disk is a vm disk and not a template disk:
    isVmDown() -- done inside ImagesHandler
  someOtherValidations()
else
  if disk is a vm disk and not a template disk:
    isVmDown()



new code:
if disk is a vm disk and not a template disk:
  isVmDown()

if diskStorageType == image
  someValidations()
  someOtherValidations()


there is a slight difference that the vm state is checked earlier, before all 
the other image validations, but IMHO it's a good change:
1. It's cheap to check, so it can save expensive database access if it fails 
fast
2. it's a use-action to fix
Line 84:                 disk.getDiskStorageType() == DiskStorageType.IMAGE ?
Line 85:                 canRemoveDiskBasedOnImageStorageCheck() : true;
Line 86:     }
Line 87: 


Line 81: 
Line 82:         return acquireLockInternal() &&
Line 83:                 validateAllVmsForDiskAreDown() &&
Line 84:                 disk.getDiskStorageType() == DiskStorageType.IMAGE ?
Line 85:                 canRemoveDiskBasedOnImageStorageCheck() : true;
yeah, I was getting a bit lazy, will fix.
Line 86:     }
Line 87: 
Line 88:     private boolean validateAllVmsForDiskAreDown() {
Line 89:         if (disk.getVmEntityType() == VmEntityType.VM) {


--
To view, visit http://gerrit.ovirt.org/11013
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1a23e1f8d129bf19c1d217c7b9201dc17a2d2c0
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to