Maor Lipchuk has posted comments on this change. Change subject: core: Add a query for attached Storage Domains ......................................................................
Patch Set 16: (8 comments) 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 will be done 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 This should be removed 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); This should be removed 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 in block domain you can import several Storage Domains, file is only one path 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? no, it is the same scenario as we do when we add a new block domain, it should be handled as it handles today in the GUI 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 replaced 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? there is no different behaviour 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? 1. I prefer that it will be more implicit, I've already verified this with that. 2. because you are trying to import it, it should be already verified in the import process at a different query. It should not happened 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 <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: [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
