Liron Aravot has posted comments on this change. Change subject: core: Add vdc query for attached Storage Domain ......................................................................
Patch Set 1: (4 comments) http://gerrit.ovirt.org/#/c/36447/1/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 29: private Guid vdsId; Line 30: protected List<StorageDomainStatic> storageDomainsWithAttachedStoragePoolId = new ArrayList<>(); Line 31: Line 32: public Guid setVdsForConnectStorage() { Line 33: vdsId = getParameters().getVdsId(); > It is the same logic we use now for attach, the query will simply fail to e that's other issue, you should prevent the issues as possible and not to cause them. this query should use host that is UP (generally it should have been the spm, as we are on the way to remove it..i can leave with any host that is UP). if the host isn't up there's no point in wasting time (for example - non responsive host). Line 34: if (vdsId == null) { Line 35: // Get a Host which is at UP state to connect to the Storage Domain. Line 36: List<VDS> hosts = Line 37: getDbFacade().getVdsDao().getAllForStoragePoolAndStatus(getParameters().getId(), VDSStatus.Up); Line 33: vdsId = getParameters().getVdsId(); Line 34: if (vdsId == null) { Line 35: // Get a Host which is at UP state to connect to the Storage Domain. Line 36: List<VDS> hosts = Line 37: getDbFacade().getVdsDao().getAllForStoragePoolAndStatus(getParameters().getId(), VDSStatus.Up); > This is the same logic done today in the attach, no need to change it there's no need to copy the code when it's already in a utility Line 38: if (!hosts.isEmpty()) { Line 39: vdsId = hosts.get(new Random().nextInt(hosts.size())).getId(); Line 40: log.info("vds id '{}' was chosen to fetch the Storage domain info", vdsId); Line 41: } else { Line 35: // Get a Host which is at UP state to connect to the Storage Domain. Line 36: List<VDS> hosts = Line 37: getDbFacade().getVdsDao().getAllForStoragePoolAndStatus(getParameters().getId(), VDSStatus.Up); Line 38: if (!hosts.isEmpty()) { Line 39: vdsId = hosts.get(new Random().nextInt(hosts.size())).getId(); This is a bug regardless of my last comment to use RandomUtils, if you'll try to get the list.size() element in the list you'll get indexOutofboundsException. Line 40: log.info("vds id '{}' was chosen to fetch the Storage domain info", vdsId); Line 41: } else { Line 42: log.warn("There is no available vds in UP state to fetch the Storage domain info from VDSM"); Line 43: return null; Line 132: log.error("Could not get Storage Domain info for Storage Domain (name:'{}', id:'{}') with VDS '{}'. ", Line 133: storageDomain.getName(), Line 134: storageDomain.getId(), Line 135: getVdsId()); Line 136: } else { > I prefer to leave it that way, it is better then nothing just log for each case Line 137: log.error("Could not get Storage Domain info with VDS '{}'. ", getVdsId()); Line 138: } Line 139: } -- To view, visit http://gerrit.ovirt.org/36447 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I749cad223d90dcc8d193765ed842d7e062ad566c Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[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
