Gilad Chaplik has posted comments on this change. Change subject: db: Numa support database implementation ......................................................................
Patch Set 21: Code-Review-1 (14 comments) please see inline. http://gerrit.ovirt.org/#/c/26996/21/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsCpuStatisticsDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsCpuStatisticsDAO.java: Line 28: * the vds cpu statistics to be saved Line 29: * @param vdsId Line 30: * the vds id that the cpus belong to Line 31: */ Line 32: void massSaveCpuStatistics(List<CpuStatistics> vdsCpuStatistics, Guid vdsId); +1 Line 33: Line 34: /** Line 35: * Update the cpu statistics data using a more efficient method Line 36: * to update all of them at once, rather than each at a time. Line 47: * Line 48: * @param vdsId Line 49: * the vds id Line 50: */ Line 51: void removeAllCpuStatisticsByVdsId(Guid vdsId); that should be handled by delete cascade, nop? how do you handle adding CPUs to host after reboot? Line 52: http://gerrit.ovirt.org/#/c/26996/21/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsCpuStatisticsDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsCpuStatisticsDAODbFacadeImpl.java: Line 46: MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() Line 47: .addValue("vds_id", vdsId); Line 48: Line 49: getCallsHandler().executeModification("DeleteVdsCpuStatisticsByVdsId", parameterSource); Line 50: } see last comment Line 51: Line 52: private MapSqlParameterSource createCpuStatisticsParametersMapper(CpuStatistics stat) { Line 53: return getCustomMapSqlParameterSource() Line 54: .addValue("cpu_core_id", stat.getCpuId()) http://gerrit.ovirt.org/#/c/26996/21/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDynamicDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsDynamicDAODbFacadeImpl.java: Line 260: .addValue("supported_emulated_machines", vds.getSupportedEmulatedMachines()) Line 261: .addValue("kdump_status", vds.getKdumpStatus().getAsNumber()) Line 262: .addValue("auto_numa_balancing", vds.getAutoNumaBalancing().getValue()) Line 263: .addValue("vds_numa_node_count", Line 264: vds.getNumaNodeList() == null ? 0 : vds.getNumaNodeList().size()) remove. Line 265: .addValue("is_numa_supported", vds.isNumaSupport()); Line 266: Line 267: return parameterSource; Line 268: } http://gerrit.ovirt.org/#/c/26996/21/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsNumaNodeDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VdsNumaNodeDAO.java: Line 9: * <code>VdsNumaNodeDAO</code> defines a type for performing CRUD operations on instances of {@link VdsNumaNode}. Line 10: * Line 11: * Line 12: */ Line 13: public interface VdsNumaNodeDAO extends DAO { this iface accommodates iface VmNumaNodeDao as well. please remove -i 'VDS' from signatures and comments. Line 14: Line 15: /** Line 16: * Get all numa nodes of a vds by vds id Line 17: * @param vdsId Line 28: * the vds numa nodes to be saved Line 29: * @param vdsId Line 30: * the vds id that the numa nodes belong to Line 31: */ Line 32: void massSaveVdsNumaNode(List<VdsNumaNode> vdsNumaNodes, Guid vdsId); plural Line 33: Line 34: /** Line 35: * Update the statistics data of the given list of vds numa nodes using a more efficient method Line 36: * to update all of them at once, rather than each at a time. Line 37: * Line 38: * @param nodes Line 39: * the vds numa nodes to be updated Line 40: */ Line 41: void massUpdateVdsNumaNodeStatistics(List<VdsNumaNode> vdsNumaNodes); plural Line 42: Line 43: /** Line 44: * Update non-statistics data of the given list of vds numa nodes using a more efficient method Line 45: * to update all of them at once, rather than each at a time. Line 46: * Line 47: * @param nodes Line 48: * the vds numa nodes to be updated Line 49: */ Line 50: void massUpdateVdsNumaNode(List<VdsNumaNode> vdsNumaNodes); plural Line 51: Line 52: /** Line 53: * Remove all the vds numa node of a given vds Line 54: * Line 63: * Line 64: * @param vdsNumaNodeIds Line 65: * the vds numa node ids to be removed Line 66: */ Line 67: void massRemoveVdsNumaNodeByVdsNumaNodeId(List<Guid> vdsNumaNodeIds); plural Line 68: http://gerrit.ovirt.org/#/c/26996/21/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmNumaNodeDAO.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmNumaNodeDAO.java: Line 10: * <code>VmNumaNodeDAO</code> defines a type for performing CRUD operations on instances of {@link VmNumaNode}. Line 11: * Line 12: * Line 13: */ Line 14: public interface VmNumaNodeDAO extends DAO { should extend vdsNumaNodeDao. see comment there. Line 15: Line 16: /** Line 17: * Get all numa nodes of a vm by vm id Line 18: * @param vmId http://gerrit.ovirt.org/#/c/26996/21/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmNumaNodeDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmNumaNodeDAODbFacadeImpl.java: Line 14: import org.ovirt.engine.core.compat.Guid; Line 15: import org.springframework.jdbc.core.RowMapper; Line 16: import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; Line 17: Line 18: public class VmNumaNodeDAODbFacadeImpl extends BaseDAODbFacade implements VmNumaNodeDAO { a lot of code duplication with vdsDaoImpl class, please handle. Line 19: Line 20: @Override Line 21: public List<VmNumaNode> getAllVmNumaNodeByVmId(Guid vmId) { Line 22: MapSqlParameterSource parameterSource = getCustomMapSqlParameterSource() http://gerrit.ovirt.org/#/c/26996/21/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmStaticDAODbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmStaticDAODbFacadeImpl.java: Line 103: .addValue("serial_number_policy", vm.getSerialNumberPolicy() == null ? null : vm.getSerialNumberPolicy().getValue()) Line 104: .addValue("custom_serial_number", vm.getCustomSerialNumber()) Line 105: .addValue("is_boot_menu_enabled", vm.isBootMenuEnabled()) Line 106: .addValue("numatune_mode", vm.getNumaTuneMode().getValue()) Line 107: .addValue("vm_numa_node_count", vm.getvNumaNodeList() == null ? 0 : vm.getvNumaNodeList().size()); -1, who said vNumaNodeList() is filled? Line 108: } Line 109: Line 110: @Override Line 111: public void remove(Guid id) { http://gerrit.ovirt.org/#/c/26996/21/packaging/dbscripts/upgrade/03_05_0330_add_numa_tables_and_columns.sql File packaging/dbscripts/upgrade/03_05_0330_add_numa_tables_and_columns.sql: Line 52: vds_numa_node_id UUID, Line 53: vds_numa_node_index SMALLINT, Line 54: is_pinned BOOLEAN DEFAULT false NOT NULL, Line 55: CONSTRAINT pk_vm_vds_numa_node_map PRIMARY KEY(id), Line 56: CONSTRAINT fk_vm_vds_numa_node_map_vds_numa_node FOREIGN KEY(vds_numa_node_id) REFERENCES numa_node(numa_node_id) ON DELETE SET NULL, > @Gilad, as you said, if vm migrate to another host with the same numa topol Leave it for now, and mark that as a known issue needs handling in bug phase. Line 57: CONSTRAINT fk_vm_vds_numa_node_map_vm_numa_node FOREIGN KEY(vm_numa_node_id) REFERENCES numa_node(numa_node_id) ON DELETE CASCADE Line 58: ); Line 59: Line 60: -- Create partial index for numa node map Line 90: PERFORM fn_db_add_column('vm_static', 'vm_numa_node_count', 'smallint'); Line 91: Line 92: -- Add columns in table vds_dynamic Line 93: PERFORM fn_db_add_column('vds_dynamic', 'auto_numa_balancing', 'smallint'); Line 94: PERFORM fn_db_add_column('vds_dynamic', 'vds_numa_node_count', 'smallint'); this field can be removed. IMO, except this comment, this file is ready. please don't change without consulting me. WELL DONE!!! Line 95: PERFORM fn_db_add_column('vds_dynamic', 'is_numa_supported', 'boolean'); Line 96: Line 97: END; $procedure$ Line 98: LANGUAGE plpgsql; -- To view, visit http://gerrit.ovirt.org/26996 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2507c084aa214bcfb65e860b11ed7dcf02af50cc Gerrit-PatchSet: 21 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Xiaolei Shi <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Jason Liao <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Xiaolei Shi <[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
