Mike Kolesnik has posted comments on this change.
Change subject: engine: Internal VmNic Information (WIP)
......................................................................
Patch Set 2: (13 inline comments)
....................................................
File backend/manager/dbscripts/create_tables.sql
Line 899: id UUID NOT NULL,
Line 900: interface_name VARCHAR(50),
Line 901: ipv4_addresses text,
Line 902: ipv6_addresses text,
Line 903: CONSTRAINT PK_vm_interface_dynamic PRIMARY KEY(id)
There should also be a FK constraint from this table to the vm_interfaces table
Line 904: ) WITH OIDS;
Line 905:
Line 906:
Line 907: --
----------------------------------------------------------------------
Line 901: ipv4_addresses text,
Line 902: ipv6_addresses text,
Line 903: CONSTRAINT PK_vm_interface_dynamic PRIMARY KEY(id)
Line 904: ) WITH OIDS;
Line 905:
You should put this in an upgrade file, this file is only used when installing
a new instance of the system, not upgrading an existing one.
Line 906:
Line 907: --
----------------------------------------------------------------------
Line 908: -- vm_interface_statistics table
Line 909: --
----------------------------------------------------------------------
....................................................
File backend/manager/dbscripts/network_sp.sql
Line 562:
Line 563:
Line 564: ----------------------------------------------------------------
Line 565: -- [vm_interface_dynamic] Table
Line 566: --
You're missing an "insert" STP
Line 567:
Line 568:
Line 569: Create or replace FUNCTION
GetVmInterfaceDynamicByVmInterfaceDynamicId(v_id UUID)
Line 570: RETURNS SETOF vm_interface_dynamic
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmInterfaceDynamic.java
Line 1: package org.ovirt.engine.core.common.businessentities;
Line 2:
Line 3: import org.ovirt.engine.core.compat.Guid;
Line 4:
Line 5: public class VmInterfaceDynamic extends IVdcQueryable implements
BusinessEntity<Guid>{
Why do you extend IVdcQueryable ?
Line 6:
Line 7: /**
Line 8: * The VmInterfaceDynamic's ID uniquely identifies a
VmInterfaceDynamic in the system.
Line 9: */
Line 9: */
Line 10: private Guid id;
Line 11:
Line 12: /**
Line 13: * The internal nic's name
I would specify that it is how the guest sees it.
Line 14: */
Line 15: String interfaceName;
Line 16:
Line 17: /**
Line 16:
Line 17: /**
Line 18: * The vNic's IPv4 addresses
Line 19: */
Line 20: String ipv4Addresses;
Why String and not List<org.ovirt.engine.core.utils.IPAddress> (or some other
class to represent the IP)?
Line 21:
Line 22: /**
Line 23: * The vNic's IPv6 addresses
Line 24: */
Line 21:
Line 22: /**
Line 23: * The vNic's IPv6 addresses
Line 24: */
Line 25: String ipv6Addresses;
Same question..
Line 26:
Line 27: public Guid getId() {
Line 28: return id;
Line 29: }
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/dbbroker/DbFacade.java
Line 879: * Returns the singleton instance of {@link
VmInterfaceDynamicDao}.
Line 880: *
Line 881: * @return the dao
Line 882: */
Line 883: public VmInterfaceDynamicDao getVmInterfaceDynamic() {
Should be called getVmInterfaceDynamicDao
Line 884: return getDao(VmInterfaceDynamicDao.class);
Line 885: }
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmInterfaceDynamicDaoDbFacadeImpl.java
Line 14: {
Line 15:
Line 16: public VmInterfaceDynamicDaoDbFacadeImpl() {
Line 17: super("VmInterfaceDynamic");
Line 18: setProcedureNameForGetAll("GetAllFromVmInterfaceDynamic");
I didn't see that you added this method
Line 19: }
Line 20:
Line 21: @Override
Line 22: protected MapSqlParameterSource
createFullParametersMapper(VmInterfaceDynamic entity) {
Line 20:
Line 21: @Override
Line 22: protected MapSqlParameterSource
createFullParametersMapper(VmInterfaceDynamic entity) {
Line 23: return getCustomMapSqlParameterSource()
Line 24: .addValue("id", entity.getId())
Please indent properly
Line 25: .addValue("interface_name", entity.getInterfaceName())
Line 26: .addValue("ipv4_addresses", entity.getIpv4Addresses())
Line 27: .addValue("ipv6_addresses", entity.getIpv6Addresses());
Line 28: }
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmInterfaceDynamicDao.java
Line 3: import org.ovirt.engine.core.common.businessentities.VmInterfaceDynamic;
Line 4: import org.ovirt.engine.core.compat.Guid;
Line 5:
Line 6: /**
Line 7: * <code>VmInterfaceDynamicDAO</code> defines a type for performing
CRUD operations on instances of
Name does not correspond with class name, I would either use @link or no
javadoc at all (since it doesnt really say anything meaningful)
Line 8: * {@link VmInterfaceDynamic}.
Line 9: */
Line 10: public interface VmInterfaceDynamicDao extends
GenericDao<VmInterfaceDynamic, Guid>, MassOperationsDao<VmInterfaceDynamic,
Guid> {
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmNetworkInterfaceDAODbFacadeImpl.java
Line 49: @Override
Line 50: public VmNetworkInterface get(Guid id) {
Line 51: MapSqlParameterSource parameterSource =
getCustomMapSqlParameterSource().addValue("id", id);
Line 52: VmNetworkInterface entity =
getCallsHandler().executeRead("Getvm_interfaceById", mapper, parameterSource);
Line 53:
entity.setVmInterfaceDynamic(DbFacade.getInstance().getVmInterfaceDynamic().get(id));
This should be done as part of mapper logic, and the fields added to the
vm_interface_view, no need to double query.
Line 54: return entity;
Line 55: }
Line 56:
Line 57: @Override
Line 120: MapSqlParameterSource parameterSource =
getCustomMapSqlParameterSource()
Line 121: .addValue("id", id);
Line 122:
Line 123: getCallsHandler().executeModification("Deletevm_interface",
parameterSource);
Line 124: DbFacade.getInstance().getVmInterfaceDynamic().remove(id);
I disagree that this is the right place for it, it's hiding this logic in an
unrelated place, I think this call needs to be done explicitly where the remove
is called (as is the case for statistics)
Line 125: }
Line 126:
Line 127: @Override
Line 128: public List<VmNetworkInterface> getAll() {
--
To view, visit http://gerrit.ovirt.org/9424
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I89ffbbb96826c5dca8e4c9cdcddbe122c3523960
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Muli Salem <msa...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Muli Salem <msa...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
