Moti Asayag has posted comments on this change. Change subject: engine: introducing vfsConfigNetworks and vfsConfigLabels tables ......................................................................
Patch Set 7: Code-Review-1 (15 comments) http://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 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 Line 31: Line 32: public Guid getNicId() { Line 33: return nicId; Line 34: } http://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 please complete missing doc 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 Line 30: * Line 31: * @param vfsConfigId Line 32: * host nic vfs config id Line 33: * label http://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. 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 here 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 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 the instance before calling this method. 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 doesn't belong to this class specifically and deserves its own class and testing. This DAO will make use at the HostNicVfsConfigNetworkDao 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 ? 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 stored procedure names) Line 120: createNetworkParametersMapper(vfsConfigId, networkId)); Line 121: } Line 122: Line 123: private void massNetworksUpdate(Guid vfsConfigId, Set<Guid> networks) { Line 146: .addValue("network_id", networkId); Line 147: return parameterSource; Line 148: } Line 149: Line 150: // VfsConfigLabels please add a specific dao for that entity. there is too much logic that doesn't belong to this class specifically and deserves its own class and testing. This DAO will make use at the HostNicVfsConfigNetworkLabelDao Line 151: Line 152: Set<String> getLabelsByVfsConfigId(Guid vfsConfigId) { Line 153: return new HashSet<String>(getCallsHandler().executeReadList("GetLabelsByVfsConfigId", Line 154: new SingleColumnRowMapper<String>(), http://gerrit.ovirt.org/#/c/36100/7/packaging/dbscripts/network_sp.sql File packaging/dbscripts/network_sp.sql: please replace tabs with spaces 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 readability (given proper indentation) 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 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 http://gerrit.ovirt.org/36100 To unsubscribe, visit http://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 <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Moti Asayag <[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
