Liron Aravot has posted comments on this change. Change subject: core: Add a query for attached Storage Domains ......................................................................
Patch Set 16: (8 comments) partial review http://gerrit.ovirt.org/#/c/34233/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsWithAttachedStoragePoolGuidQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/GetStorageDomainsWithAttachedStoragePoolGuidQuery.java: Line 30: import org.ovirt.engine.core.common.vdscommands.VDSParametersBase; Line 31: import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; Line 32: import org.ovirt.engine.core.compat.Guid; Line 33: Line 34: public class GetStorageDomainsWithAttachedStoragePoolGuidQuery<P extends StorageDomainsAndStoragePoolIdQueryParameters> extends QueriesCommandBase<P> { I think that this query should be seperated to two different queries, right now there's too much code going on here and it's determined by the parameters which flow should be executed. If there's a shared logic it can reside in a helper or parent class. Line 35: private Guid vdsId = null; Line 36: Line 37: public GetStorageDomainsWithAttachedStoragePoolGuidQuery(P parameters) { Line 38: super(parameters); Line 31: import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; Line 32: import org.ovirt.engine.core.compat.Guid; Line 33: Line 34: public class GetStorageDomainsWithAttachedStoragePoolGuidQuery<P extends StorageDomainsAndStoragePoolIdQueryParameters> extends QueriesCommandBase<P> { Line 35: private Guid vdsId = null; no need for the = null, you can remove it when you rebase Line 36: Line 37: public GetStorageDomainsWithAttachedStoragePoolGuidQuery(P parameters) { Line 38: super(parameters); Line 39: } Line 43: // Get a Host which is at UP state to connect to the Storage Domain. Line 44: List<VDS> hosts = Line 45: getDbFacade().getVdsDao().getAllForStoragePoolAndStatus(getParameters().getId(), VDSStatus.Up); Line 46: if (!hosts.isEmpty()) { Line 47: vdsId = hosts.get(new Random().nextInt(hosts.size())).getId(); 1. please use RandomUtils.instance().pickRandom(hosts); Line 48: log.info("vds id '{}' was chosen to fetch the Storage domain info", vdsId); Line 49: } else { Line 50: log.warn("There is no available vds in UP state to fetch the Storage domain info from VDSM"); Line 51: return; Line 66: return getParameters().getStorageDomainList() != null; Line 67: } Line 68: Line 69: private boolean isFileDomain() { Line 70: return getParameters().getStorageServerConnection() != null; why for file domain we get a single connection and for block domains a list? Line 71: } Line 72: Line 73: private List<StorageDomainStatic> getBlockStorageDomains() { Line 74: VDSReturnValue vdsReturnValue = null; Line 69: private boolean isFileDomain() { Line 70: return getParameters().getStorageServerConnection() != null; Line 71: } Line 72: Line 73: private List<StorageDomainStatic> getBlockStorageDomains() { in that case you don't disconnect? Line 74: VDSReturnValue vdsReturnValue = null; Line 75: List<StorageDomain> storageDomains = getParameters().getStorageDomainList(); Line 76: List<StorageDomainStatic> storageDomainsWithAttachedStoragePoolId = new ArrayList<>(); Line 77: Line 81: try { Line 82: boolean connectResult = connectStorageDomain(storageDomain); Line 83: if (connectResult) { Line 84: vdsReturnValue = Line 85: getVdsBroker().RunVdsCommand(VDSCommandType.HSMGetStorageDomainInfo, you added runVdsCommand method Line 86: new HSMGetStorageDomainInfoVDSCommandParameters(vdsId, storageDomain.getId())); Line 87: } Line 88: } catch (RuntimeException e) { Line 89: logErrorMessage(storageDomain); Line 88: } catch (RuntimeException e) { Line 89: logErrorMessage(storageDomain); Line 90: continue; Line 91: } Line 92: if (!vdsReturnValue.getSucceeded()) { why there's a different behavior between exception and a failure? Line 93: logErrorMessage(storageDomain); Line 94: return null; Line 95: } Line 96: Line 128: } Line 129: } Line 130: Line 131: getBackend().runInternalAction(VdcActionType.DisconnectStorageServerConnection, Line 132: new StorageServerConnectionParametersBase(getParameters().getStorageServerConnection(), vdsId)); 1. why not disconnect using StorageHelperDirector? 2. where do you ensure that this connection isn't somehow used by other domain? disconnect in that case will cause to the host to not access the existing domain anymore. Line 133: } Line 134: return storageDomains; Line 135: } Line 136: -- To view, visit http://gerrit.ovirt.org/34233 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6fc7a2d722611b1bccecee9868223ff437596ed Gerrit-PatchSet: 16 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
