Allon Mureinik has posted comments on this change.

Change subject: core: add ability edit NFS path in webadmin
......................................................................


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

(5 inline comments)

Please see my and Ayal's comment inline

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageServerConnectionCommand.java
Line 96:     @Override
Line 97:     protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 98:         //lock the path to NFS to avoid at the same time if some other 
user tries to:
Line 99:         // add new storage domain to same path or edit another storage 
server connection to point to same path
Line 100:         return 
Collections.singletonMap(getParameters().getStorageServerConnection().getconnection(),
 LockMessagesMatchUtil.STORAGE_CONNECTION);
Note to self: revisit.
Line 101:     }


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 94:         return super.canDoAction();
Line 95:     }
Line 96: 
Line 97: 
Line 98:     protected String createDomainNamesList(List<StorageDomain> 
domains) {
LinqUtils.aggregate should do the trick
Line 99:              //Build domain names list to display in the error
Line 100:             StringBuilder domainNames = new StringBuilder();
Line 101:             for(StorageDomain domain : domains) {
Line 102:                 domainNames.append(domain.getStorageName());


Line 108:     }
Line 109: 
Line 110:     protected boolean isConnectionEditable(StorageDomain 
storageDomain) {
Line 111:         boolean isEditable =
Line 112:                 (storageDomain.getStorageDomainType() == 
StorageDomainType.Data || storageDomain.getStorageDomainType() == 
StorageDomainType.Master)
Why not just add StorageDomainType.isData() ?
Line 113:                         && storageDomain.getStatus() == 
StorageDomainStatus.Maintenance;
Line 114:         return isEditable;
Line 115:     }
Line 116: 


Line 119:         StoragePoolIsoMap map = getStoragePoolIsoMap();
Line 120: 
Line 121:         
changeStorageDomainStatusInTransaction(map,StorageDomainStatus.Locked);
Line 122:         //connect to the new path
Line 123:         boolean hasConnectStorageSucceeded = connectToStorage();
The one passed to the function from the UI, like all storage operations
Line 124:         VDSReturnValue returnValueUpdatedStorageDomain = null;
Line 125:         //update info such as free space - because we switched to a 
different server
Line 126:         if(!hasConnectStorageSucceeded) {
Line 127:            setSucceeded(false);


Line 127:            setSucceeded(false);
Line 128:            VdcFault f = new VdcFault();
Line 129:            f.setError(VdcBllErrors.StorageServerConnectionError);
Line 130:            getReturnValue().setFault(f);
Line 131:            
changeStorageDomainStatusInTransaction(map,StorageDomainStatus.Maintenance);
Just revert the compensation
Line 132:            return;
Line 133:         }
Line 134: 
Line 135:         returnValueUpdatedStorageDomain = updateStatsForDomain();


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifaff5344ff191d6bdf53cc706c7bb796167a56b3
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to