Allon Mureinik has posted comments on this change.
Change subject: core: add ability edit NFS path in webadmin
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(20 inline comments)
Please see inline comments.
Most of them are nitpicking, but there are some "real" issues here.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommand.java
Line 43: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE);
Line 44: }
Line 45:
Line 46: // Check if the NFS path has a valid format
Line 47: if (!new
NfsMountPointConstraint().isValid(newConnectionDetails.getconnection(), null)) {
This is not the correct way to use constraints.
You should just slap @ValidNFSMountPoint on the relevant data member
Line 48: return
failCanDoAction(VdcBllMessages.VALIDATION_STORAGE_CONNECTION_INVALID);
Line 49: }
Line 50:
Line 51: // Check if connection exists by id - otherwise there's
nothing to update
Line 59:
Line 60: // Check that there is no other connection with the new
suggested path
Line 61: List<StorageServerConnections> connections =
Line 62:
getStorageConnDao().getAllForStorage(newConnectionDetails.getconnection());
Line 63: if (connections != null) {
the DAO ensures that you get an empty list, not null, if there are no
connection, so this is redundant.
Line 64: if (connections.size() > 1) {
Line 65: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 66: }
Line 67: else if (connections.size() == 1 &&
!connections.get(0).getid().equals(oldConnection.getid())) {
Line 61: List<StorageServerConnections> connections =
Line 62:
getStorageConnDao().getAllForStorage(newConnectionDetails.getconnection());
Line 63: if (connections != null) {
Line 64: if (connections.size() > 1) {
Line 65: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
This isn't the correct error message - your domain has several connections, not
a connection with several domains.
Also - please explain why this is a problem?
Line 66: }
Line 67: else if (connections.size() == 1 &&
!connections.get(0).getid().equals(oldConnection.getid())) {
Line 68: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
Line 69: }
Line 63: if (connections != null) {
Line 64: if (connections.size() > 1) {
Line 65: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 66: }
Line 67: else if (connections.size() == 1 &&
!connections.get(0).getid().equals(oldConnection.getid())) {
The else can be ommitted, as the if branch does not terminate properly, but
it's a matter of taste I guess.
Line 68: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS);
Line 69: }
Line 70:
Line 71: }
Line 70:
Line 71: }
Line 72:
Line 73: List<StorageDomain> domains =
getStorageDomainsByConnId(newConnectionDetails.getid());
Line 74: if (domains != null) {
here too, the check is redundant.
Line 75: if (domains.size() != 1) {
Line 76: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS);
Line 77: }
Line 78: setStorageDomain(domains.get(0));
Line 97: //connect to the new path
Line 98: boolean hasConnectStorageSucceeded = connectToStorage();
Line 99:
Line 100: //update info such as free space - because we switched to a
different server
Line 101: VDSReturnValue returnValueUpdatedStorageDomain =
updateStatsForDomain();
If the connect failed, there is no reason to proceed to do this (heavy)
operation - you should just early return before it.
Line 102:
Line 103: if (hasConnectStorageSucceeded &&
returnValueUpdatedStorageDomain.getSucceeded()) {
Line 104: final StorageDomain updatedStorageDomain =
(StorageDomain) returnValueUpdatedStorageDomain.getReturnValue();
Line 105: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
Line 132: return runVdsCommand(VDSCommandType.GetStorageDomainStats,
Line 133: new
GetStorageDomainStatsVDSCommandParameters(getVds().getId(),
getStorageDomain().getId()));
Line 134: }
Line 135:
Line 136:
please remove the redundant blank line
Line 137:
Line 138: protected StorageServerConnectionDAO getStorageConnDao() {
Line 139: return getDbFacade().getStorageServerConnectionDao();
Line 140: }
Line 136:
Line 137:
Line 138: protected StorageServerConnectionDAO getStorageConnDao() {
Line 139: return getDbFacade().getStorageServerConnectionDao();
Line 140: }
I'd group all the getXXXDao() methods together
Line 141:
Line 142: protected ConnectStorageServerVDSCommandParameters
createParametersForVdsm(Guid vdsmId,
Line 143: Guid storagePoolId,
Line 144: StorageType storageType,
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/UpdateStorageServerConnectionCommandTest.java
Line 36:
Line 37: @RunWith(MockitoJUnitRunner.class)
Line 38: public class UpdateStorageServerConnectionCommandTest {
Line 39:
Line 40: private LockManager lockManager = new InMemoryLockManager();
Unless I'm missing something, this is never used.
If so - can you please remove it?
If not - please explain where/how it's used.
Line 41:
Line 42: @Rule
Line 43: public MockEJBStrategyRule ejbRule = new MockEJBStrategyRule();
Line 44:
Line 38: public class UpdateStorageServerConnectionCommandTest {
Line 39:
Line 40: private LockManager lockManager = new InMemoryLockManager();
Line 41:
Line 42: @Rule
can this perhaps be a @ClassRule ?
Line 43: public MockEJBStrategyRule ejbRule = new MockEJBStrategyRule();
Line 44:
Line 45: private
UpdateStorageServerConnectionCommand<StorageServerConnectionParametersBase>
command = null;
Line 46:
Line 43: public MockEJBStrategyRule ejbRule = new MockEJBStrategyRule();
Line 44:
Line 45: private
UpdateStorageServerConnectionCommand<StorageServerConnectionParametersBase>
command = null;
Line 46:
Line 47: private StorageServerConnections newConnection = null;
Not a big fan of the explicit "= null" initializations, but I guess that too is
a matter of taste.
Line 48:
Line 49: @Mock
Line 50: private StorageServerConnectionDAO storageConnDao;
Line 51:
Line 69: StorageServerConnections oldConnection =
createConnection(id,"multipass.my.domain.tlv.company.com:/export/allstorage/data1",StorageType.NFS,
NfsVersion.V4,50,0);
Line 70:
when(storageConnDao.get(newConnection.getid())).thenReturn(oldConnection);
Line 71: }
Line 72:
Line 73: private StorageServerConnections createConnection(Guid id, String
connection, StorageType type, NfsVersion version, int timeout, int retrans) {
this can be static
Line 74: StorageServerConnections connectionDetails = new
StorageServerConnections();
Line 75: connectionDetails.setid(id.toString());
Line 76: connectionDetails.setconnection(connection);
Line 77: connectionDetails.setNfsVersion(version);
Line 74: StorageServerConnections connectionDetails = new
StorageServerConnections();
Line 75: connectionDetails.setid(id.toString());
Line 76: connectionDetails.setconnection(connection);
Line 77: connectionDetails.setNfsVersion(version);
Line 78: connectionDetails.setNfsTimeo((short)timeout);
why not define it as short in the method's signature?
Line 79: connectionDetails.setstorage_type(type);
Line 80: connectionDetails.setNfsRetrans((short)retrans);
Line 81: return connectionDetails;
Line 82: }
Line 76: connectionDetails.setconnection(connection);
Line 77: connectionDetails.setNfsVersion(version);
Line 78: connectionDetails.setNfsTimeo((short)timeout);
Line 79: connectionDetails.setstorage_type(type);
Line 80: connectionDetails.setNfsRetrans((short)retrans);
here too
Line 81: return connectionDetails;
Line 82: }
Line 83:
Line 84: @Test
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/StorageServerConnectionDAOTest.java
Line 166: @Test
Line 167: public void testUpdateNfsServerConnection() {
Line 168: //create a new connection
Line 169: StorageServerConnections newNFSServerConnection = new
StorageServerConnections();
Line 170:
newNFSServerConnection.setid("0cb136e8-e5ed-472b-8914-260bc48c2987");
please move this constant to FixturesTools.java
Line 171: newNFSServerConnection.setstorage_type(StorageType.NFS);
Line 172: newNFSServerConnection.setconnection("host/lib/data");
Line 173: newNFSServerConnection.setNfsVersion(NfsVersion.V4);
Line 174: newNFSServerConnection.setNfsRetrans((short) 0);
....................................................
Commit Message
Line 6:
Line 7: core: add ability edit NFS path in webadmin
Line 8:
Line 9: Add the option to edit NFS path and the overriden
Line 10: additional options of storage domain (retransmissions, timeout,
version)
s/options/mount options/
Line 11: in webadmin UI --> Storage tab.
Line 12:
Line 13: Editing those properties is available only when
Line 14: the storage domain is in maintenance status and is data/master domain,
Line 9: Add the option to edit NFS path and the overriden
Line 10: additional options of storage domain (retransmissions, timeout,
version)
Line 11: in webadmin UI --> Storage tab.
Line 12:
Line 13: Editing those properties is available only when
IMHO s/those/these
Line 14: the storage domain is in maintenance status and is data/master domain,
Line 15: and for storage of nfs type. These are the only properties which are
Line 16: editable in this status.
Line 17:
Line 10: additional options of storage domain (retransmissions, timeout,
version)
Line 11: in webadmin UI --> Storage tab.
Line 12:
Line 13: Editing those properties is available only when
Line 14: the storage domain is in maintenance status and is data/master domain,
"...and is a data/master domain"
Line 15: and for storage of nfs type. These are the only properties which are
Line 16: editable in this status.
Line 17:
Line 18: Editing is still available for storage domains in status Active/Mixed,
but
Line 18: Editing is still available for storage domains in status Active/Mixed,
but
Line 19: only for name and description - like it was before this patch.
Line 20:
Line 21: related to bug
Line 22: https://bugzilla.redhat.com/show_bug.cgi?id=835543
In order for the build scripts to catch this, it should be in the commit
message footer:
Related-To: https://bugzilla.redhat.com/show_bug.cgi?id=835543
Change-Id: Ifaff5344ff191d6bdf53cc706c7bb796167a56b3
Signed-off-by: Alissa Bonas <[email protected]>
Line 23:
Line 24: Change-Id: Ifaff5344ff191d6bdf53cc706c7bb796167a56b3
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java
Line 1082: boolean isEditAvailable;
Line 1083: boolean isActive =
storageDomain.getStorageDomainSharedStatus() == StorageDomainSharedStatus.Active
Line 1084: || storageDomain.getStorageDomainSharedStatus() ==
StorageDomainSharedStatus.Mixed;
Line 1085: boolean isInMaintenance = (storageDomain.getStatus() ==
StorageDomainStatus.Maintenance);
Line 1086: boolean isDataDomain = (storageDomain.getStorageDomainType()
== StorageDomainType.Data) || (storageDomain.getStorageDomainType() ==
StorageDomainType.Master);
This "logic" repeats itself in several places.
Consider extracting something like StorageDomainType.isData
Line 1087: boolean isBlockStorage =
storageDomain.getStorageType().isBlockDomain();
Line 1088:
Line 1089: isEditAvailable = isActive || isBlockStorage || (
isInMaintenance && isDataDomain) ;
Line 1090: return isEditAvailable;
--
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: 6
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: 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
