Ayal Baron has posted comments on this change.
Change subject: core: add ability edit NFS path in webadmin
......................................................................
Patch Set 7: (17 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/LockMessagesMatchUtil.java
Line 24: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED.name());
Line 25: public static final Pair<String, String> STORAGE = new
Pair<String, String>(LockingGroup.STORAGE.name(),
Line 26: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED.name());
Line 27: public static final Pair<String, String> STORAGE_CONNECTION = new
Pair<String, String>(LockingGroup.STORAGE_CONNECTION.name(),
Line 28: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED.name());
*not relevant to this patch*
assuming for a moment this class is necessary, why is the code here not an
automatic loop that goes over the enums in LockingGroup and creates the static
pairs according to the names found there? there is nothing here that requires
manual code writing (it's just code bloat and error prone).
Also, seeing as the message is *always* the same even for gluster (which means
the 'what if' use case here has been proven to be not relevant), seems totally
redundant to have a pair versus a string? and since it's a constant string then
I'd revisit above assumption and say this entire class is redundant and we just
need to replace LockMessagesMatchUtil with LockingGroup
e.g.:
return Collections.singletonMap(cluster.getId().toString(),
LockingGroup.REGISTER_VDS);
Line 29: public static final Pair<String, String> REGISTER_VDS = new
Pair<String, String>(LockingGroup.REGISTER_VDS.name(),
Line 30: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED.name());
Line 31: public static final Pair<String, String> VM_SNAPSHOTS = new
Pair<String, String>(LockingGroup.VM_SNAPSHOTS.name(),
Line 32: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED.name());
....................................................
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);
I'm assuming that for NFS, local and Posix getconnection is returning the path?
What is returned for gLuster? block (iSCSI) ?
Line 101: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 78: }
Line 79: }
Line 80:
Line 81: List<StorageDomain> domains =
getStorageDomainsByConnId(newConnectionDetails.getid());
Line 82: if (domains.size() != 1) {
1. what if there are no storage domains associated with the connection? (I'm
assuming this is possible via rest api?)
2. with block domains this will be problematic as you can definitely have cases
with multiple domains for same connection
Line 83: String domainNames = createDomainNamesList(domains);
Line 84: addCanDoActionMessage(String.format("$domainNames %1$s",
domainNames));
Line 85: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 86: }
Line 94: return super.canDoAction();
Line 95: }
Line 96:
Line 97:
Line 98: protected String createDomainNamesList(List<StorageDomain>
domains) {
seems like this code structure is repeated many times and should have been
solved in a more generic way by now
(e.g. override to string of the objects in the list and return a string with
joiner [1], or a more generic way of passing the method to call on each object
in the list and return that)
[1]
http://google-collections.googlecode.com/svn/trunk/javadoc/com/google/common/base/Joiner.html
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)
Again, not for this patch but how about implementing:
type = storageDomain.getStorageDomainType()
is(type).in(StorageDomainType.Data, StorageDomainType.Master) ?
We repeat this kind of check many times in the code and I think it could pretty
things up
Here's an example: http://stackoverflow.com/questions/7390571/java-in-operator
Line 113: && storageDomain.getStatus() ==
StorageDomainStatus.Maintenance;
Line 114: return isEditable;
Line 115: }
Line 116:
Line 117: @Override
Line 118: protected void executeCommand() {
Line 119: StoragePoolIsoMap map = getStoragePoolIsoMap();
Line 120:
Line 121:
changeStorageDomainStatusInTransaction(map,StorageDomainStatus.Locked);
space after comma?
Line 122: //connect to the new path
Line 123: boolean hasConnectStorageSucceeded = connectToStorage();
Line 124: VDSReturnValue returnValueUpdatedStorageDomain = null;
Line 125: //update info such as free space - because we switched to a
different server
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();
which host is this using to connect?
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 121:
changeStorageDomainStatusInTransaction(map,StorageDomainStatus.Locked);
Line 122: //connect to the new path
Line 123: boolean hasConnectStorageSucceeded = connectToStorage();
Line 124: VDSReturnValue returnValueUpdatedStorageDomain = null;
Line 125: //update info such as free space - because we switched to a
different server
1. this comment seems misplaced
2. we're updating the connection info, what does that have to do with the
storage domain statistics?
3. all this is assuming that there is a 1:1 mapping between connection and
storage domain. This is fine for file based domains but not for block storage.
With the initial check for NFS this wouldn't be a problem, but I wonder if
this isn't an indication that we shouldn't have these ops here?
Line 126: if(!hasConnectStorageSucceeded) {
Line 127: setSucceeded(false);
Line 128: VdcFault f = new VdcFault();
Line 129: f.setError(VdcBllErrors.StorageServerConnectionError);
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);
space after comma?
Line 132: return;
Line 133: }
Line 134:
Line 135: returnValueUpdatedStorageDomain = updateStatsForDomain();
Line 135: returnValueUpdatedStorageDomain = updateStatsForDomain();
Line 136:
Line 137: if (returnValueUpdatedStorageDomain.getSucceeded()) {
Line 138: final StorageDomain updatedStorageDomain =
(StorageDomain) returnValueUpdatedStorageDomain.getReturnValue();
Line 139: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
since you've created a helper method for this, why not use it here as well?
Line 140: @Override
Line 141: public Void runInTransaction() {
Line 142:
getStorageConnDao().update(getParameters().getStorageServerConnection());
Line 143:
getStorageDomainDynamicDao().update(updatedStorageDomain.getStorageDynamicData());
Line 146: });
Line 147:
Line 148: setSucceeded(true);
Line 149: }
Line 150:
changeStorageDomainStatusInTransaction(map,StorageDomainStatus.Maintenance);
1. space after comma
2. why are you doing this with compensation here? seems redundant
Line 151: }
Line 152:
Line 153: protected StoragePoolIsoMap getStoragePoolIsoMap() {
Line 154: StoragePoolIsoMapId mapId = new
StoragePoolIsoMapId(getStorageDomain().getId(),
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
Line 1: package org.ovirt.engine.core.bll.storage;
I did not review this file
Line 2:
Line 3: import org.junit.Before;
Line 4: import org.junit.ClassRule;
Line 5: import org.junit.Test;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 205: UpdateStoragePool(958,
ActionGroup.EDIT_STORAGE_POOL_CONFIGURATION, QuotaDependency.NONE),
Line 206: FenceVdsManualy(959, ActionGroup.MANIPUTLATE_HOST, false,
QuotaDependency.NONE),
Line 207: AddExistingNFSStorageDomain(960,
ActionGroup.CREATE_STORAGE_DOMAIN, QuotaDependency.NONE),
Line 208: AddStorageServerConnection(1000,
ActionGroup.CREATE_STORAGE_DOMAIN, QuotaDependency.NONE),
Line 209:
UpdateStorageServerConnection(1001,ActionGroup.CREATE_STORAGE_DOMAIN,QuotaDependency.NONE),
spaces
Line 210: DisconnectStorageServerConnection(1002,
ActionGroup.CREATE_STORAGE_DOMAIN, QuotaDependency.NONE),
Line 211: RemoveStorageServerConnection(1003,
ActionGroup.CREATE_STORAGE_DOMAIN, QuotaDependency.NONE),
Line 212: ConnectHostToStoragePoolServers(1004, QuotaDependency.NONE),
Line 213: DisconnectHostFromStoragePoolServers(1005, QuotaDependency.NONE),
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/StorageServerConnectionDAOTest.java
Line 2:
Line 3: import static org.junit.Assert.assertEquals;
Line 4: import static org.junit.Assert.assertFalse;
Line 5: import static org.junit.Assert.assertNotNull;
Line 6: import static org.junit.Assert.assertNotSame;
I did not review this file
Line 7: import static org.junit.Assert.assertNull;
Line 8: import static org.junit.Assert.assertTrue;
Line 9:
Line 10: import java.util.List;
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java
Line 368: IStorageModel item = null;
Line 369: switch (storage.getStorageType()) {
Line 370: case NFS:
Line 371: item = PrepareNfsStorageForEdit(storage);
Line 372: boolean isPathAndOverrideOptsEditable =
isNfsPathEditable(storage);
what is the purpose of this boolean? it's name is longer than the original and
not more descriptive imo.
s/isPathAndOver.*/isConnEditable/? (iiuc that is actually what we'd like to
know here)
Line 373: isStorageEditable = isStorageEditable ||
isPathAndOverrideOptsEditable;
Line 374: break;
Line 375:
Line 376: case FCP:
Line 432: {
Line 433: final NfsStorageModel model = new NfsStorageModel();
Line 434: model.setRole(storage.getStorageDomainType());
Line 435:
Line 436: boolean isPathAndOverrideOptsEditable =
isNfsPathEditable(storage);
same
Line 437:
Line 438: model.getPath().setIsChangable(isPathAndOverrideOptsEditable);
Line 439:
model.getOverride().setIsChangable(isPathAndOverrideOptsEditable);
Line 440:
Line 471: return model;
Line 472: }
Line 473:
Line 474: private boolean isNfsPathEditable(StorageDomain storage) {
Line 475: return (storage.getStorageDomainType() ==
StorageDomainType.Data || storage.getStorageDomainType() ==
StorageDomainType.Master) && storage.getStatus() ==
StorageDomainStatus.Maintenance;
a master domain is always a data domain (underneath).
I'm not sure why it was an either or implementation here, but instead of the
'in' operator I suggested before you could just add a helper method
(isDataDomain) to the StorageDomainType enum that would return true if type is
either master or data.
git grep StorageDomainType.Data shows a lot of results and my guess is that
wherever only it is checked without checking Master as well is a bug.
While at it (different patch), you could add an isMaster to replace all the
"getStorageDomainType() == StorageDomainType.Master" in the code.
Line 476: }
Line 477:
Line 478: private IStorageModel PrepareLocalStorageForEdit(StorageDomain
storage)
Line 479: {
--
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 <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Itamar Heim <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
