Allon Mureinik has posted comments on this change. Change subject: core: send domains map on connectStoragePool ......................................................................
Patch Set 2: (7 comments) http://gerrit.ovirt.org/#/c/22712/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ActivateStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ActivateStorageDomainCommand.java: Line 73: changeStorageDomainStatusInTransaction(map, StorageDomainStatus.Locked); Line 74: freeLock(); Line 75: Line 76: log.infoFormat("ActivateStorage Domain. Before Connect all hosts to pool. Time:{0}", new Date()); Line 77: connectAllHostsToPool(); // TBD: really needed? so what's the verdict? Line 78: runVdsCommand(VDSCommandType.ActivateStorageDomain, Line 79: new ActivateStorageDomainVDSCommandParameters(getStoragePool().getId(), getStorageDomain().getId())); Line 80: log.infoFormat("ActivateStorage Domain. After Connect all hosts to pool. Time:{0}", new Date()); Line 81: http://gerrit.ovirt.org/#/c/22712/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConntectVDSToPoolAndDomains.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/ConntectVDSToPoolAndDomains.java: Line 10: import org.ovirt.engine.core.utils.log.Log; Line 11: import org.ovirt.engine.core.utils.log.LogFactory; Line 12: import org.ovirt.engine.core.vdsbroker.ResourceManager; Line 13: Line 14: public class ConntectVDSToPoolAndDomains extends ActivateDeactivateSingleAsyncOperation { // To be removed probably so what's the verdict? Line 15: Line 16: private static Log log = LogFactory.getLog(ConntectVDSToPoolAndDomains.class); Line 17: Line 18: public ConntectVDSToPoolAndDomains(ArrayList<VDS> vdss, StorageDomain domain, StoragePool storagePool) { http://gerrit.ovirt.org/#/c/22712/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java: Line 1583: MaxNumOfTriesToRunFailedAutoStartVm, Line 1584: Line 1585: @TypeConverterAttribute(Boolean.class) Line 1586: @DefaultValueAttribute("true") Line 1587: StoragePoolNoMetadata, Consider "StoragePoolWithoutMetadata" Line 1588: Line 1589: Invalid; http://gerrit.ovirt.org/#/c/22712/2/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ConnectStoragePoolVDSCommandParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/ConnectStoragePoolVDSCommandParameters.java: Line 13: Line 14: private boolean refreshOnly; Line 15: Line 16: public ConnectStoragePoolVDSCommandParameters() { Line 17: } Not sure this is really required - but I wouldn't mess with it right now. Line 18: Line 19: public ConnectStoragePoolVDSCommandParameters(VDS vds, StoragePool storagePool) { Line 20: this.vds = vds; Line 21: this.storagePool = storagePool; http://gerrit.ovirt.org/#/c/22712/2/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ConnectStoragePoolVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ConnectStoragePoolVDSCommand.java: Line 19: } Line 20: Line 21: private static StorageDomain getMasterStorageDomain(Guid storagePoolId) { Line 22: return DbFacade.getInstance().getStorageDomainDao() Line 23: .getStorageDomainByTypeAndPool(storagePoolId, StorageDomainType.Master); why can't we pass the master SD object on the parameter object instead of accessing the DB here. It's kind of code-smell to call DbFacade from a VDS command. [not saying it can't be done, or even isn't done elsewhere, but still - it's a code smell] Line 24: } Line 25: Line 26: private boolean isPoolWithoutMetadata() { Line 27: return FeatureSupported.storagePoolNoMetadata(getParameters().getStoragePool().getcompatibility_version()); Line 28: } Line 29: Line 30: public void connectStoragePool() { Line 31: Map<String, String> storageDomains = null; Line 32: Guid masterDomainId = getMasterStorageDomain(getParameters().getStoragePoolId()).getId(); // performance issue? See above comment about DB access - but if you must, you can just use DBFacade.getStorageDomainDAO().getMasterStorageDomainIdForPool(poolId) Line 33: Line 34: if (isPoolWithoutMetadata()) { Line 35: storageDomains = StoragePoolDomainHelper.buildStoragePoolDomainsMap( Line 36: DbFacade.getInstance().getStoragePoolIsoMapDao().getAllForStoragePool( Line 65: } Line 66: Line 67: // dont throw exception on errors except StoragePoolMasterNotFound for Line 68: // master domain failure treatment Line 69: protected void proceedConnectProxyReturnValue() { This breaks the general contract of using proceedProxyReturnValue() after executeVdsBrokerCommand(). I'd prefer having something like this: proceedProxyReturnValue() { if (isRefreshing) { // same logic as in the execute proceedRefreshProxyReturnValue(); } else { proceedConnectProxyReturnValue() } } Line 70: VdcBllErrors returnStatus = getReturnValueFromStatus(getReturnStatus()); Line 71: switch (returnStatus) { Line 72: case Done: Line 73: case StoragePoolMasterNotFound: -- To view, visit http://gerrit.ovirt.org/22712 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic16034c6757959e5d0445814a8acc2221a74f6aa Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
