Allon Mureinik has posted comments on this change.
Change subject: core,restapi: attach detach storage connections
......................................................................
Patch Set 17: Code-Review-1
(12 comments)
See comments/questions inline.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetLunsByVgIdQuery.java
Line 26:
Line 27: @Override
Line 28: protected void executeQueryCommand() {
Line 29: List<LUNs> luns = getLunsForVgId(getVgId());
Line 30: List<LUNs> nonDummyLuns = new ArrayList<LUNs>();
Initialize this with the size of "luns"
Line 31: StorageType storageType = getStorageType(luns);
Line 32: Map<String, LUNs> lunsFromDeviceMap =
getLunsFromDeviceMap(storageType);
Line 33:
Line 34: for (LUNs lun : luns) {
Line 32: Map<String, LUNs> lunsFromDeviceMap =
getLunsFromDeviceMap(storageType);
Line 33:
Line 34: for (LUNs lun : luns) {
Line 35: // Filter dummy luns
Line 36: if
(lun.getLUN_id().startsWith(BusinessEntitiesDefinitions.DUMMY_LUN_ID_PREFIX)) {
I'd move this logic to the LUNs class:
public boolean isDummy() {
return getLUN_id().startsWith(BusinessEntitiesDefinitions.DUMMY_LUN_ID_PREFIX);
}
Line 37: continue;
Line 38: }
Line 39: nonDummyLuns.add(lun);
Line 40:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AttachStorageConnectionToStorageDomainCommand.java
Line 14: import org.ovirt.engine.core.utils.transaction.TransactionMethod;
Line 15: import org.ovirt.engine.core.utils.transaction.TransactionSupport;
Line 16:
Line 17: @NonTransactiveCommandAttribute(forceCompensation = true)
Line 18: public class AttachStorageConnectionToStorageDomainCommand<T extends
AttachDetachStorageConnectionParameters>
what about locks?
Line 19: extends StorageDomainCommandBase<T> {
Line 20:
Line 21: public AttachStorageConnectionToStorageDomainCommand(T parameters)
{
Line 22: super(parameters);
Line 30: ||
!validate(storageConnectionValidator.isDomainOfConnectionExistsAndInactive(getStorageDomain()))
Line 31: ||
!validate(storageConnectionValidator.isISCSIConnectionAndDomain(getStorageDomain())))
{
Line 32: return false;
Line 33: }
Line 34: if
(storageConnectionValidator.isConnectionForISCSIDomainAttached(getStorageDomain()))
{
why doesn't this return a ValidationResult too?
Line 35: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_FOR_DOMAIN_ALREADY_EXISTS);
Line 36: }
Line 37: return true;
Line 38: }
Line 52: // Create storage server connection mapping
Line 53: final LUN_storage_server_connection_map connectionMapRecord =
Line 54: new
LUN_storage_server_connection_map(dummyLun.getLUN_id(),
getParameters().getStorageConnectionId());
Line 55:
Line 56: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
why not just have the whole command transactive?
Line 57: @Override
Line 58: public Void runInTransaction() {
Line 59: List<StorageServerConnections> connectionsForDomain =
null;
Line 60:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/DetachStorageConnectionFromStorageDomainCommand.java
Line 12: import org.ovirt.engine.core.common.errors.VdcBllMessages;
Line 13: import org.ovirt.engine.core.utils.transaction.TransactionMethod;
Line 14: import org.ovirt.engine.core.utils.transaction.TransactionSupport;
Line 15:
Line 16: @NonTransactiveCommandAttribute(forceCompensation = true)
what about locks?
Line 17: public class DetachStorageConnectionFromStorageDomainCommand<T extends
AttachDetachStorageConnectionParameters>
Line 18: extends StorageDomainCommandBase<T> {
Line 19:
Line 20: public DetachStorageConnectionFromStorageDomainCommand(T
parameters) {
Line 29: ||
!validate(storageConnectionValidator.isDomainOfConnectionExistsAndInactive(getStorageDomain()))
Line 30: ||
!validate(storageConnectionValidator.isISCSIConnectionAndDomain(getStorageDomain())))
{
Line 31: return false;
Line 32: }
Line 33:
if(!storageConnectionValidator.isConnectionForISCSIDomainAttached(getStorageDomain()))
{
why doesn't this return a ValidationResult too?
Line 34: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_CONNECTION_FOR_DOMAIN_NOT_EXIST);
Line 35: }
Line 36: return true;
Line 37: }
Line 50: List<LUNs> lunsForVG =
getLunDao().getAllForVolumeGroup(getStorageDomain().getStorage());
Line 51: final Collection<LUNs> lunsToRemove =
Line 52: (Collection<LUNs>)
CollectionUtils.intersection(lunsForConnection, lunsForVG);
Line 53:
Line 54: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
why not just have the whole command transactive?
Line 55: @Override
Line 56: public Void runInTransaction() {
Line 57: for (LUNs lun : lunsToRemove) {
Line 58: if (getLunDao().get(lun.getLUN_id()) != null) {
Line 54: TransactionSupport.executeInNewTransaction(new
TransactionMethod<Void>() {
Line 55: @Override
Line 56: public Void runInTransaction() {
Line 57: for (LUNs lun : lunsToRemove) {
Line 58: if (getLunDao().get(lun.getLUN_id()) != null) {
The get here is redundant.
Worse case scenario - you'd run a DELETE statement that removes nothing.
It's still faster than SELECT+DELETE.
Line 59: getLunDao().remove(lun.getLUN_id());
Line 60: }
Line 61: }
Line 62: return null;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageDomainCommandBase.java
Line 224: return DbFacade.getInstance().getLunDao();
Line 225: }
Line 226:
Line 227: public static void proceedLUNInDb(final LUNs lun, StorageType
storageType, String volumeGroupId) {
Line 228: if (DbFacade.getInstance().getLunDao().get(lun.getLUN_id())
== null) {
why no extract a getLunDao() method?
Line 229: lun.setvolume_group_id(volumeGroupId);
Line 230: DbFacade.getInstance().getLunDao().save(lun);
Line 231: } else if (!volumeGroupId.isEmpty()){
Line 232:
DbFacade.getInstance().getLunDao().updateLUNsVolumeGroupId(lun.getLUN_id(),
volumeGroupId);
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageConnectionValidator.java
Line 13: import org.ovirt.engine.core.dal.dbbroker.DbFacade;
Line 14: import org.ovirt.engine.core.dao.StorageServerConnectionDAO;
Line 15:
Line 16: public class StorageConnectionValidator {
Line 17: protected static final String STORAGE_DOMAIN_NAME_REPLACEMENT =
"$domainNames %1$s";
why not private?
Line 18:
Line 19: private StorageServerConnections connection;
Line 20:
Line 21: public StorageConnectionValidator(StorageServerConnections
connection) {
Line 69:
Line 70: return ValidationResult.VALID;
Line 71: }
Line 72:
Line 73: public boolean isConnectionForISCSIDomainAttached(StorageDomain
storageDomain) {
why not a ValidationResult?
Line 74: List<StorageServerConnections> connectionsForDomain =
getAllConnectionsForDomain(storageDomain.getId());
Line 75: for (StorageServerConnections connectionForDomain :
connectionsForDomain) {
Line 76: if
(connectionForDomain.getid().equals(connection.getid())) {
Line 77: return true;
--
To view, visit http://gerrit.ovirt.org/17864
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I319bc9f51c81e2df0c7ed3b3338fcd7b4783fe84
Gerrit-PatchSet: 17
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
