Ayal Baron has posted comments on this change.

Change subject: core: avoid having SD active on vdsm and not in engine
......................................................................


Patch Set 3: (3 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ReconstructMasterDomainCommand.java
Line 36: @NonTransactiveCommandAttribute(forceCompensation = true)
Line 37: public class ReconstructMasterDomainCommand<T extends 
ReconstructMasterParameters> extends
Line 38:         DeactivateStorageDomainCommand<T> {
Line 39: 
Line 40:     protected boolean reconstructOpsSucceeded;
it's reconstructOp not Ops.
Line 41:     /**
Line 42:      * Constructor for command creation when compensation is applied 
on startup
Line 43:      *
Line 44:      * @param commandId


Line 133:         try {
Line 134:             reconstructOpsSucceeded = reconstructMaster();
Line 135:             connectAndRefreshAllUpHosts(reconstructOpsSucceeded);
Line 136: 
Line 137:             if (!_isLastMaster && reconstructOpsSucceeded) {
couldn't have connectAndRefreshAllUpHosts disconnected the storage and induce 
failure of updateVmInSpm?
Line 138:                 SearchParameters p = new SearchParameters(
Line 139:                     MessageFormat.format(DesktopsInStoragePoolQuery,
Line 140:                         getStoragePool().getname()), SearchType.VM);
Line 141: 


Line 217:                                 
returnValue.getVdsError().getMessage());
Line 218:                     }
Line 219:                 }
Line 220:                 // only if we deactivate the storage domain we want 
to disconnect from it.
Line 221:                 if (!getParameters().isInactive()) {
You might get here if _isLastMaster is true and commandSucceeded. Iiuc though, 
you should not disconnect if commandSucceeded is true.

In fact, I'm not sure what this piece of code is doing here at all.  It seems 
logically unrelated to the above part, it doesn't correspond with the method 
name and I'm not clear on why we'd want to disconnect to begin with.
Line 222:                     StorageHelperDirector.getInstance()
Line 223:                             
.getItem(getStorageDomain().getstorage_type())
Line 224:                             
.DisconnectStorageFromDomainByVdsId(getStorageDomain(), vds.getId());
Line 225:                 }


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida51b027b8078555ffab5b80d2ffe54572fa6cca
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@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