Liron Ar has posted comments on this change. Change subject: core: add StorageDomainOvfInfo entity/db ......................................................................
Patch Set 10: (9 comments) http://gerrit.ovirt.org/#/c/23461/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageDomainOvfInfo.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/StorageDomainOvfInfo.java: Line 10: private Guid ovfDiskId; Line 11: private boolean updated; Line 12: Line 13: public StorageDomainOvfInfo(Guid storageDomainId, List<Guid> storedOvfIds, Line 14: Guid ovfDiskId, boolean updated) { > Did you use the formatter here? Done Line 15: this.storageDomainId = storageDomainId; Line 16: this.storedOvfIds = storedOvfIds; Line 17: this.ovfDiskId = ovfDiskId; Line 18: this.updated = updated; Line 72: Line 73: if (updated != ovfInfo.updated) return false; Line 74: if (ovfDiskId != null ? !ovfDiskId.equals(ovfInfo.ovfDiskId) : ovfInfo.ovfDiskId != null) return false; Line 75: if (storageDomainId != null ? !storageDomainId.equals(ovfInfo.storageDomainId) : ovfInfo.storageDomainId != null) Line 76: return false; > please use ObjectUtils auto generated code, changed Line 77: Line 78: return true; Line 79: } Line 80: Line 82: public int hashCode() { Line 83: int result = storageDomainId != null ? storageDomainId.hashCode() : 0; Line 84: result = 31 * result + (ovfDiskId != null ? ovfDiskId.hashCode() : 0); Line 85: result = 31 * result + (updated ? 1 : 0); Line 86: return result; > please use ObjectUtils can you elaborate? what in objectutils? Line 87: } Line 88: Line 89: @Override Line 90: public String toString() { http://gerrit.ovirt.org/#/c/23461/10/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageDomainOvfInfoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageDomainOvfInfoDbFacadeImpl.java: Line 43: toReturn.setUpdated(resultSet.getBoolean("ovf_updated")); Line 44: toReturn.setOvfDiskId(getGuid(resultSet, "ovf_disk_id")); Line 45: String storedOvfs = resultSet.getString("stored_ovfs_ids"); Line 46: if (storedOvfs != null && !storedOvfs.isEmpty()) { Line 47: toReturn.setStoredOvfIds(GuidUtils.getGuidListFromStringArray(Arrays.asList(StringUtils.split(resultSet.getString("stored_ovfs_ids"), ',')))); > Why not just use GuidUtils.getGuidListFromString ? Done Line 48: } else { Line 49: toReturn.setStoredOvfIds(new LinkedList<Guid>()); Line 50: } Line 51: return toReturn; Line 68: } Line 69: Line 70: @Override Line 71: public List<StorageDomainOvfInfo> getAll() { Line 72: throw new NotImplementedException(); > why? because it's not implemented, no need to implement it as it's never used. Line 73: } Line 74: Line 75: @Override Line 76: protected MapSqlParameterSource createIdParameterMapper(Guid guid) { Line 73: } Line 74: Line 75: @Override Line 76: protected MapSqlParameterSource createIdParameterMapper(Guid guid) { Line 77: return getCustomMapSqlParameterSource().addValue("storage_domain_id", guid); > why not the existing GuidMapper? GuidMapper is a row mapper for reading the rows from the resultset, in this method we map the parameters for the stored procedure, so there's no use for the guidmapper. Line 78: } Line 79: Line 80: @Override Line 81: protected RowMapper<StorageDomainOvfInfo> createEntityRowMapper() { http://gerrit.ovirt.org/#/c/23461/10/packaging/dbscripts/create_functions.sql File packaging/dbscripts/create_functions.sql: Line 52: Line 53: Line 54: Line 55: Line 56: CREATE OR REPLACE FUNCTION public.fnSplitterWithSeperator(ids TEXT, seperator VARCHAR(10)) RETURNS SETOF idTextType IMMUTABLE AS > Why do we need this? Why not just always use ","? it belongs to another patch, will move it there..it's used to split the passed OVFS, as the ovfs might contain "," i used a custom separator Line 57: $function$ Line 58: BEGIN Line 59: RETURN QUERY Line 60: SELECT regexp_split_to_table(ids, seperator) AS id; Line 56: CREATE OR REPLACE FUNCTION public.fnSplitterWithSeperator(ids TEXT, seperator VARCHAR(10)) RETURNS SETOF idTextType IMMUTABLE AS Line 57: $function$ Line 58: BEGIN Line 59: RETURN QUERY Line 60: SELECT regexp_split_to_table(ids, seperator) AS id; > please use spaces instead of tabs. Done Line 61: END; $function$ Line 62: LANGUAGE plpgsql; Line 63: Line 64: http://gerrit.ovirt.org/#/c/23461/10/packaging/dbscripts/storages_sp.sql File packaging/dbscripts/storages_sp.sql: Line 290: AS $procedure$ Line 291: BEGIN Line 292: RETURN QUERY SELECT ovf.storage_domain_id Line 293: FROM storage_domains_ovf_info ovf Line 294: WHERE string_to_array(ovf.stored_ovfs_ids,',') && string_to_array(v_ovfs_ids,','); > s/&&/AND/ && is an array function to check if there are common elements between the arrays, therefore it can't be replaced with AND Line 295: END; $procedure$ Line 296: LANGUAGE plpgsql; Line 297: Line 298: -- To view, visit http://gerrit.ovirt.org/23461 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6cd52f5b9f14559bcec224dee868cf267cd15ebe Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Ar <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Ar <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Sergey Gotliv <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vered Volansky <[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
