Allon Mureinik has posted comments on this change.
Change subject: core: add canDoAction to remove storage connection
......................................................................
Patch Set 4: I would prefer that you didn't submit this
(16 inline comments)
See comments inline.
A few general clarifications:
1. Several of my comments are personal style comments - feel free to implement
or ignore as you see fit.
2. Something seems to be off with your formatter. In the project's style, we
add spaces around brackets.
E.g.: "if(someCondition){" should be changed to "if (someCondition) {".
This repeats throughout the patch.
3. The comments on
backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
apply to ALL the AppErrors files as well.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
Line 32: protected boolean canDoAction() {
Line 33: String connectionId = getConnection().getid();
Line 34: List<StorageDomain> domains = new ArrayList<>();
Line 35: if(StringUtils.isEmpty(connectionId) ) {
Line 36: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY);
IMHO, this can be done with an annotation on the connection object - no need to
repeat this code in all the commands
Line 37: }
Line 38: StorageServerConnections connection =
getStorageServerConnectionDao().get(connectionId);
Line 39: StorageType storageType = connection.getstorage_type();
Line 40: if(storageType.isFileDomain()) {
Line 34: List<StorageDomain> domains = new ArrayList<>();
Line 35: if(StringUtils.isEmpty(connectionId) ) {
Line 36: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY);
Line 37: }
Line 38: StorageServerConnections connection =
getStorageServerConnectionDao().get(connectionId);
Why do I need this as well as getParameters().getConnection()?
If the connection object on the parameters cannot be trusted, perhaps it should
be refactored to just have a connection ID?
Line 39: StorageType storageType = connection.getstorage_type();
Line 40: if(storageType.isFileDomain()) {
Line 41: //go to storage domain static, get all storage domains
where storage field = storage connection id
Line 42: domains = getStorageDomainsByConnId(connectionId);
Line 46: }
Line 47: }
Line 48: else if(storageType.equals(StorageType.ISCSI)) {
Line 49: List<String> domainNames = new ArrayList<>();
Line 50: List<String> diskNames = new ArrayList<>();
Java 7!
yey!
(Although IMHO a LinkedList would be more appropriate here)
Line 51: //go to luns to storage connections map table, get it from
there
Line 52: List<LUNs> luns =
getLunDao().getAllForStorageServerConnection(connectionId);
Line 53: if(luns.size() > 0) {
Line 54: String volumeGroupId = null;
Line 49: List<String> domainNames = new ArrayList<>();
Line 50: List<String> diskNames = new ArrayList<>();
Line 51: //go to luns to storage connections map table, get it from
there
Line 52: List<LUNs> luns =
getLunDao().getAllForStorageServerConnection(connectionId);
Line 53: if(luns.size() > 0) {
I'd use if (!luns.isEmpty())
Line 54: String volumeGroupId = null;
Line 55: for(LUNs lun : luns) {
Line 56: volumeGroupId = lun.getvolume_group_id();
Line 57: if(StringUtils.isNotEmpty(volumeGroupId)) {
Line 53: if(luns.size() > 0) {
Line 54: String volumeGroupId = null;
Line 55: for(LUNs lun : luns) {
Line 56: volumeGroupId = lun.getvolume_group_id();
Line 57: if(StringUtils.isNotEmpty(volumeGroupId)) {
I think a positively phrased condition will be easier to understand
Line 58: // non empty vg id indicates there's a storage
domain using the lun
Line 59: String domainName = lun.getStorageDomainName();
Line 60: domainNames.add(domainName);
Line 61: }
Line 65: diskNames.add(lunDiskName);
Line 66: }
Line 67: }
Line 68: String domainNamesForMessage = null;
Line 69: if(domainNames.size() > 0 ) {
I'd use !domainNames.isEmpty()
Line 70: // Build domain names list to display in the error
Line 71: domainNamesForMessage =
prepareEntityNamesForMessage(domainNames);
Line 72: if(diskNames.size() < 1) {
Line 73: return
prepareFailureMessageForDomains(domainNamesForMessage) ;
Line 68: String domainNamesForMessage = null;
Line 69: if(domainNames.size() > 0 ) {
Line 70: // Build domain names list to display in the error
Line 71: domainNamesForMessage =
prepareEntityNamesForMessage(domainNames);
Line 72: if(diskNames.size() < 1) {
I'd use diskNames.isEmpty()
Line 73: return
prepareFailureMessageForDomains(domainNamesForMessage) ;
Line 74: }
Line 75: else {
Line 76: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 71: domainNamesForMessage =
prepareEntityNamesForMessage(domainNames);
Line 72: if(diskNames.size() < 1) {
Line 73: return
prepareFailureMessageForDomains(domainNamesForMessage) ;
Line 74: }
Line 75: else {
I'd unwrap this
Line 76: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 77: return
prepareFailureMessageForDomainsAndDisks(domainNamesForMessage,diskNamesForMessage);
Line 78: }
Line 79: }
Line 76: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 77: return
prepareFailureMessageForDomainsAndDisks(domainNamesForMessage,diskNamesForMessage);
Line 78: }
Line 79: }
Line 80: else if(diskNames.size() > 0 ) {
I'd use !diskNames.isEmpty()
Line 81: String diskNamesForMessage =
prepareEntityNamesForMessage(diskNames);
Line 82: return
prepareFailureMessageForDisks(diskNamesForMessage);
Line 83: }
Line 84:
Line 99: domainNamesForMessage.append(",");
Line 100: }
Line 101: // Remove the last "," after the last domain
Line 102:
domainNamesForMessage.deleteCharAt(domainNamesForMessage.length() - 1);
Line 103: return domainNamesForMessage.toString();
This entire function should be changed to a simple
StringUtils.join(entityNames, ",")
Line 104: }
Line 105:
Line 106: @Override
Line 107: protected void executeCommand() {
Line 132: protected Map<String, Pair<String, String>> getExclusiveLocks() {
Line 133: return
Collections.singletonMap(getConnection().getconnection(),
Line 134:
LockMessagesMatchUtil.makeLockingPair(LockingGroup.STORAGE_CONNECTION,
Line 135:
VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED));
Line 136: }
Please move this method either up or done, so all the CDA message related
methods are grouped together
Line 137:
Line 138: protected String
createDomainNamesListFromStorageDomains(List<StorageDomain> domains) {
Line 139: // Build domain names list to display in the error
Line 140: StringBuilder domainNames = new StringBuilder();
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommandTest.java
Line 28: import static org.mockito.Mockito.when;
Line 29:
Line 30: @RunWith(MockitoJUnitRunner.class)
Line 31: public class RemoveStorageServerConnectionCommandTest {
Line 32: @ClassRule
Somethign's funky with the indentation here
Line 33: public static MockEJBStrategyRule ejbRule = new
MockEJBStrategyRule();
Line 34:
Line 35: private RemoveStorageServerConnectionCommand command = null;
Line 36:
Line 86: Guid id = Guid.NewGuid();
Line 87: StorageServerConnections connectionDetails =
populateBasicConnectionDetails(id, connection, type);
Line 88: connectionDetails.setNfsVersion(version);
Line 89: connectionDetails.setNfsTimeo((short) timeout);
Line 90: connectionDetails.setNfsRetrans((short) retrans);
why cast here? why not define reateNFSConnection's params as shorts?
Line 91: return connectionDetails;
Line 92: }
Line 93:
Line 94: private StorageServerConnections createIscsiConnection(String
connection,
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
Line 453: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS,
Line 454:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE,
Line 455:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS,
Line 456:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS,
Line 457: ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL__DISKS,
You have a double "_" here - please fix
Line 458:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_WRONG_PARAMETERS_FOR_STORAGE_TYPE,
Line 459:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_CHANGE_STORAGE_TYPE,
Line 460: ACTION_TYPE_FAILED_STORAGE_DOMAIN_STATUS_ILLEGAL,
Line 461: ACTION_TYPE_FAILED_STORAGE_DOMAIN_STATUS_ILLEGAL2,
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 343: ACTION_TYPE_FAILED_STORAGE_CONNECTION_NOT_EXIST=Cannot ${action}
${type}. Storage connection doesn't exist.
Line 344: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY=Cannot ${action}
${type}. Storage connection id is empty.
Line 345: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS=Cannot ${action}
${type}. Storage connection already exists.
Line 346:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE=Cannot
${action} ${type}. Storage connection parameters can be edited only for NFS
data domains in maintenance.
Line 347:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS=Cannot
${action} ${type}. Cannot ${action} ${type}. Storage connection parameters are
related to the following storage domains : ${domainNames}.
You have a double "Cannot ${action} ${type}" - please remove one.
Also, IMHO, "used by" would be better than "related to"
Line 348:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS=Cannot
${action} ${type}. Storage connection parameters are related to the following
storage domains : ${domainNames} and disks: ${diskNames}.
Line 349:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL__DISKS=Cannot
${action} ${type}. Storage connection parameters are related to the following
disks : ${diskNames}.
Line 350:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_WRONG_PARAMETERS_FOR_STORAGE_TYPE=Cannot
${action} ${type}. Connection parameters are invalid for this storage type.
Line 351:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_CHANGE_STORAGE_TYPE=Cannot
${action} ${type}. Storage type cannot be edited.
Line 344: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ID_EMPTY=Cannot ${action}
${type}. Storage connection id is empty.
Line 345: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS=Cannot ${action}
${type}. Storage connection already exists.
Line 346:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_ACTION_FOR_STORAGE=Cannot
${action} ${type}. Storage connection parameters can be edited only for NFS
data domains in maintenance.
Line 347:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS=Cannot
${action} ${type}. Cannot ${action} ${type}. Storage connection parameters are
related to the following storage domains : ${domainNames}.
Line 348:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL_STORAGE_DOMAINS_AND_DISKS=Cannot
${action} ${type}. Storage connection parameters are related to the following
storage domains : ${domainNames} and disks: ${diskNames}.
same here regarding "related to"
Line 349:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_BELONGS_TO_SEVERAL__DISKS=Cannot
${action} ${type}. Storage connection parameters are related to the following
disks : ${diskNames}.
Line 350:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_WRONG_PARAMETERS_FOR_STORAGE_TYPE=Cannot
${action} ${type}. Connection parameters are invalid for this storage type.
Line 351:
ACTION_TYPE_FAILED_STORAGE_CONNECTION_UNSUPPORTED_CHANGE_STORAGE_TYPE=Cannot
${action} ${type}. Storage type cannot be edited.
Line 352: ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST=Cannot ${action} ${type}.
Storage Domain doesn't exist.
--
To view, visit http://gerrit.ovirt.org/15269
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib7a633b06195a7b8ca1cd4fc5023c05010a3161d
Gerrit-PatchSet: 4
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: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
