Liron Aravot has posted comments on this change.

Change subject: engine: Remove IsValidVDSCommand (Preporation for removing 
global lock on SPM ops)
......................................................................


Patch Set 2: I would prefer that you didn't submit this

(3 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IsoDomainListSyncronizer.java
Line 238:             FileTypeExtension fileTypeExt) {
Line 239:         boolean refreshSucceeded = false;
Line 240:         // Setting the indication to the indication whether the 
storage pool is valid.
Line 241:         boolean updateFromVDSMSucceeded = true;
Line 242: 
why is it done now without the check of the storage pool status?
Line 243:         // If the SPM and the storage pool are valid, try to refresh 
the Iso list by fetching it from the SPM.
Line 244:         if (fileTypeExt == FileTypeExtension.ISO) {
Line 245:             updateFromVDSMSucceeded = 
updateIsoListFromVDSM(storagePoolId, storageDomainId);
Line 246:         } else if (fileTypeExt == FileTypeExtension.Floppy) {


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java
Line 84:         try {
Line 85:             updateVmInSpm(getVm().getstorage_pool_id(),
Line 86:                     Arrays.asList(getVm()));
Line 87:         } catch (Exception e) {
Line 88:             // DO nothing
updateVmInSpm can fail for various reasons, right now the behaviour is to fail 
the actions when this operation fails as the ovf wasn't updated. can you please 
explain how this change and behaviour change is done in this patch? how about 
logging so at least we can observe what went wrong? audit log?
Line 89:         }
Line 90:         setSucceeded(true);
Line 91:     }
Line 92: 


....................................................
File 
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/CommonVmPoolWithVmsCommandTestAbstract.java
Line 272:      */
Line 273:     private storage_pool mockStoragePool() {
Line 274:         storage_pool storage_pool = new storage_pool();
Line 275:         storage_pool.setstatus(StoragePoolStatus.Up);
Line 276: 
can this line be removed?
Line 277:         return storage_pool;
Line 278:     }
Line 279: 
Line 280:     private static void setDiskList(VmTemplate vmTemplate) {


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I73c0fa0fd66f2f14132594a0b0450a7ecd7cf166
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to