Liron Ar has posted comments on this change.
Change subject: core: add ability edit NFS path in webadmin
......................................................................
Patch Set 7: (9 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
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);
there's no need to revert it, it should be done automatically.
Line 132: return;
Line 133: }
Line 134:
Line 135: returnValueUpdatedStorageDomain = updateStatsForDomain();
Line 131:
changeStorageDomainStatusInTransaction(map,StorageDomainStatus.Maintenance);
Line 132: return;
Line 133: }
Line 134:
Line 135: returnValueUpdatedStorageDomain = updateStatsForDomain();
case of failure here?
Line 136:
Line 137: if (returnValueUpdatedStorageDomain.getSucceeded()) {
Line 138: final StorageDomain updatedStorageDomain =
(StorageDomain) returnValueUpdatedStorageDomain.getReturnValue();
Line 139: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
Line 146: });
Line 147:
Line 148: setSucceeded(true);
Line 149: }
Line 150:
changeStorageDomainStatusInTransaction(map,StorageDomainStatus.Maintenance);
+1
Line 151: }
Line 152:
Line 153: protected StoragePoolIsoMap getStoragePoolIsoMap() {
Line 154: StoragePoolIsoMapId mapId = new
StoragePoolIsoMapId(getStorageDomain().getId(),
Line 161: executeInNewTransaction(new
TransactionMethod<StoragePoolIsoMap>() {
Line 162: @SuppressWarnings("synthetic-access")
Line 163: @Override
Line 164: public StoragePoolIsoMap runInTransaction() {
Line 165: CompensationContext context =
getCompensationContext();
this line can be removed
Line 166: context.snapshotEntityStatus(map, map.getstatus());
Line 167: map.setstatus(status);
Line 168: getStoragePoolIsoMapDao().updateStatus(map.getId(),
map.getstatus());
Line 169: getCompensationContext().stateChanged();
Line 171: }
Line 172: });
Line 173: }
Line 174:
Line 175: protected void executeInNewTransaction(TransactionMethod<?>
method) {
can you elaborate why do we need this method? in addition, sometimes it's used
and sometimes TransactionSupport.executeInNewTransaction is called during the
execution of this command
Line 176: TransactionSupport.executeInNewTransaction(method);
Line 177: }
Line 178:
Line 179: protected boolean connectToStorage() {
Line 199: new ArrayList<StorageServerConnections>(Arrays
Line 200: .asList(storageServerConnection)));
Line 201: return newConnectionParametersForVdsm;
Line 202: }
Line 203:
don't we have any of those method higher in the hierarchy?
Line 204: protected StorageDomainDAO getStorageDomainDao() {
Line 205: return getDbFacade().getStorageDomainDao();
Line 206: }
Line 207:
Line 223:
Line 224: @Override
Line 225: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 226: Map<String, Pair<String, String>> locks = new HashMap<String,
Pair<String, String>>();
Line 227: locks.put(getStorageDomain().getId().toString(),
LockMessagesMatchUtil.STORAGE);
perhaps we can add message specific to this command.
Line 228: // lock the path to NFS to avoid at the same time if some
other user tries to:
Line 229: // add new storage domain to same path or edit another
storage server connection to point to same path
Line 230:
locks.put(getParameters().getStorageServerConnection().getconnection(),
Line 231: LockMessagesMatchUtil.STORAGE_CONNECTION);
Line 223:
Line 224: @Override
Line 225: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 226: Map<String, Pair<String, String>> locks = new HashMap<String,
Pair<String, String>>();
Line 227: locks.put(getStorageDomain().getId().toString(),
LockMessagesMatchUtil.STORAGE);
getStorageDomain() can be null and cause to NPE.
Line 228: // lock the path to NFS to avoid at the same time if some
other user tries to:
Line 229: // add new storage domain to same path or edit another
storage server connection to point to same path
Line 230:
locks.put(getParameters().getStorageServerConnection().getconnection(),
Line 231: LockMessagesMatchUtil.STORAGE_CONNECTION);
Line 230:
locks.put(getParameters().getStorageServerConnection().getconnection(),
Line 231: LockMessagesMatchUtil.STORAGE_CONNECTION);
Line 232: //lock the old path as well so no one will delete it in the
middle of edit
Line 233: locks.put(oldConnection.getconnection(),
Line 234: LockMessagesMatchUtil.STORAGE_CONNECTION);
there's no NPE here? oldConnection at this point seems to be null
Line 235: return locks;
Line 236: }
Line 237:
Line 238: @Override
--
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
