Alona Kaplan has posted comments on this change. Change subject: engine: introducing vfsConfigNetworks and vfsConfigLabels tables ......................................................................
Patch Set 7: (14 comments) https://gerrit.ovirt.org/#/c/36100/7/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/HostNicVfsConfig.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/HostNicVfsConfig.java: Line 24: private int numOfFreeVfs; Line 25: Line 26: private boolean allNetworksAllowed; Line 27: Line 28: private Set<Guid> networks = new HashSet<>(); > please move initialization to c'tor Done Line 29: Line 30: private Set<String> labels = new HashSet<>(); Line 31: Line 32: public Guid getNicId() { Line 26: private boolean allNetworksAllowed; Line 27: Line 28: private Set<Guid> networks = new HashSet<>(); Line 29: Line 30: private Set<String> labels = new HashSet<>(); > same Done Line 31: Line 32: public Guid getNicId() { Line 33: return nicId; Line 34: } https://gerrit.ovirt.org/#/c/36100/7/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/HostNicVfsConfigDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/HostNicVfsConfigDao.java: > please add '@param' before each parameter Done Line 1: package org.ovirt.engine.core.dao.network; Line 2: Line 3: import org.ovirt.engine.core.common.businessentities.network.HostNicVfsConfig; Line 4: import org.ovirt.engine.core.compat.Guid; Line 25: */ Line 26: void removeNetwork(Guid vfsConfigId, Guid networkId); Line 27: Line 28: /** Line 29: * Attaches a network to the allowed network list of the specified vfs config > the description is wrong Done Line 30: * Line 31: * @param vfsConfigId Line 32: * host nic vfs config id Line 33: * label https://gerrit.ovirt.org/#/c/36100/7/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/HostNicVfsConfigDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/HostNicVfsConfigDaoDbFacadeImpl.java: Line 53: entity.setId(getGuidDefaultEmpty(rs, "id")); Line 54: entity.setNicId(getGuidDefaultEmpty(rs, "nic_id")); Line 55: entity.setAllNetworksAllowed(rs.getBoolean("all_networks_allowed")); Line 56: Line 57: ((HostNicVfsConfigDaoDbFacadeImpl) DbFacade.getInstance().getHostNicVfsConfigDao()).fillNetworksAndLabelsDataOnConfig(entity); > the DbFacade.getInstance() can be replaced by a call to dbFacade. HostNicVfsConfigRowMapper is static class. Can't reference to the outer class members. Line 58: Line 59: return entity; Line 60: } Line 61: } Line 59: return entity; Line 60: } Line 61: } Line 62: Line 63: @Override > why do you need to override it ? you aren't introducing any new behaviour h Done Line 64: public HostNicVfsConfig get(Guid id) { Line 65: HostNicVfsConfig vfsConfig = super.get(id); Line 66: return vfsConfig; Line 67: } Line 65: HostNicVfsConfig vfsConfig = super.get(id); Line 66: return vfsConfig; Line 67: } Line 68: Line 69: @Override > same Done Line 70: public List<HostNicVfsConfig> getAll() { Line 71: List<HostNicVfsConfig> allConfigs = super.getAll(); Line 72: return allConfigs; Line 73: } Line 72: return allConfigs; Line 73: } Line 74: Line 75: private void fillNetworksAndLabelsDataOnConfig(HostNicVfsConfig vfsConfig) { Line 76: if (vfsConfig == null) { > how can it be null ? it is private, used only once and you've instantiated Done Line 77: return; Line 78: } Line 79: Line 80: Guid id = vfsConfig.getId(); Line 106: .addValue("vfs_config_id", vfsConfigId); Line 107: return parameterSource; Line 108: } Line 109: Line 110: // VfsConfigNetworks > please add a specific dao for that entity. there is too much logic that doe host_nic_vfs_config_network/label are just m2m connector tables. I don't think they deserve their own dbFacade. The tables are not independent and used only by hostNicvfsConfigDao. Line 111: Line 112: Set<Guid> getNetworksByVfsConfigId(Guid vfsConfigId) { Line 113: return new HashSet<Guid>(getCallsHandler().executeReadList("GetNetworksByVfsConfigId", createGuidMapper(), Line 114: createVfsConfigIdParameter(vfsConfigId))); Line 108: } Line 109: Line 110: // VfsConfigNetworks Line 111: Line 112: Set<Guid> getNetworksByVfsConfigId(Guid vfsConfigId) { > package protected ? This method is used by the tests. Line 113: return new HashSet<Guid>(getCallsHandler().executeReadList("GetNetworksByVfsConfigId", createGuidMapper(), Line 114: createVfsConfigIdParameter(vfsConfigId))); Line 115: } Line 116: Line 115: } Line 116: Line 117: @Override Line 118: public void addNetwork(Guid vfsConfigId, Guid networkId) { Line 119: getCallsHandler().executeModification("InsertvfsConfigNetwork", > CamelCase for stored procedure name simplifies the read (same for the other Done Line 120: createNetworkParametersMapper(vfsConfigId, networkId)); Line 121: } Line 122: Line 123: private void massNetworksUpdate(Guid vfsConfigId, Set<Guid> networks) { https://gerrit.ovirt.org/#/c/36100/7/packaging/dbscripts/network_sp.sql File packaging/dbscripts/network_sp.sql: > please replace tabs with spaces Done Line 1: Line 2: Line 3: ---------------------------------------------------------------- Line 4: -- [network] Table Line 1387: RETURNS VOID Line 1388: AS $procedure$ Line 1389: BEGIN Line 1390: DELETE FROM vfs_config_networks Line 1391: WHERE vfs_config_id = v_vfs_config_id and network_id = v_network_id; > please break conditions into condition per line - that will improve readabi Done Line 1392: END; $procedure$ Line 1393: LANGUAGE plpgsql; Line 1394: Line 1395: Create or replace FUNCTION DeleteAllvfsConfigNetworks(v_vfs_config_id UUID) Line 1429: RETURNS VOID Line 1430: AS $procedure$ Line 1431: BEGIN Line 1432: DELETE FROM vfs_config_labels Line 1433: WHERE vfs_config_id = v_vfs_config_id and label = v_label; > same Done Line 1434: END; $procedure$ Line 1435: LANGUAGE plpgsql; Line 1436: Line 1437: Create or replace FUNCTION DeleteAllvfsConfigLabels(v_vfs_config_id UUID) -- To view, visit https://gerrit.ovirt.org/36100 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia383df7060b97c17459ef4576d300e7de217e966 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@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
