Mike Kolesnik has posted comments on this change.
Change subject: core: Add vnic profiles to DB and entities
......................................................................
Patch Set 41: (18 inline comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/network/VnicProfile.java
Line 18: private static final long serialVersionUID = 1019016330475623259L;
Line 19:
Line 20: @NotNull(groups = { UpdateEntity.class, RemoveEntity.class })
Line 21: private Guid id;
Line 22: @Pattern(regexp = "^[_a-zA-Z0-9]{1,50}$", message =
"ILLEGAL_VNIC_PROFILE_NAME", groups = { CreateEntity.class,
No need to limit size, size check already does that...
Also I found this gem that you can use:
@ValidName(message = "VALIDATION_NAME_INVALID", groups = { CreateEntity.class,
UpdateEntity.class })
Line 23: UpdateEntity.class })
Line 24: @Size(min = 1, max =
BusinessEntitiesDefinitions.VNIC_PROFILE_NAME_SIZE)
Line 25: private String name;
Line 26: @NotNull
Line 20: @NotNull(groups = { UpdateEntity.class, RemoveEntity.class })
Line 21: private Guid id;
Line 22: @Pattern(regexp = "^[_a-zA-Z0-9]{1,50}$", message =
"ILLEGAL_VNIC_PROFILE_NAME", groups = { CreateEntity.class,
Line 23: UpdateEntity.class })
Line 24: @Size(min = 1, max =
BusinessEntitiesDefinitions.VNIC_PROFILE_NAME_SIZE)
Why not limit this to groups?
Line 25: private String name;
Line 26: @NotNull
Line 27: private Guid networkId;
Line 28: private boolean portMirroring;
Line 22: @Pattern(regexp = "^[_a-zA-Z0-9]{1,50}$", message =
"ILLEGAL_VNIC_PROFILE_NAME", groups = { CreateEntity.class,
Line 23: UpdateEntity.class })
Line 24: @Size(min = 1, max =
BusinessEntitiesDefinitions.VNIC_PROFILE_NAME_SIZE)
Line 25: private String name;
Line 26: @NotNull
Why not limit this to groups?
Line 27: private Guid networkId;
Line 28: private boolean portMirroring;
Line 29: private String customProperties;
Line 30: private String description;
Line 25: private String name;
Line 26: @NotNull
Line 27: private Guid networkId;
Line 28: private boolean portMirroring;
Line 29: private String customProperties;
Wouldn't it make more sense to save this as Map<String, String>?
Line 30: private String description;
Line 31:
Line 32: @Override
Line 33: public Guid getId() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java
Line 422: NON_VM_NETWORK_CANNOT_SUPPORT_STP(ErrorType.CONFLICT),
Line 423: NETWORK_MTU_DIFFERENCES(ErrorType.CONFLICT),
Line 424: NETWORK_MTU_OVERRIDE_NOT_SUPPORTED(ErrorType.CONFLICT),
Line 425: EXTERNAL_NETWORK_CANNOT_BE_PROVISIONED(ErrorType.NOT_SUPPORTED),
Line 426: ILLEGAL_VNIC_PROFILE_NAME(ErrorType.BAD_PARAMETERS),
Not necessary if using VALIDATION_NAME_INVALID
Line 427:
ACTION_TYPE_FAILED_MIGRATION_NETWORK_IS_NOT_SUPPORTED(ErrorType.NOT_SUPPORTED),
Line 428:
ACTION_TYPE_FAILED_PROVIDER_DOESNT_EXIST(ErrorType.BAD_PARAMETERS),
Line 429:
ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_REQUIRED(ErrorType.BAD_PARAMETERS),
Line 430:
ACTION_TYPE_FAILED_MANAGEMENT_NETWORK_ADDRESS_CANNOT_BE_CHANGED(ErrorType.BAD_PARAMETERS),
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/network/VnicProfileDaoDbFacadeImpl.java
Line 16: super("VnicProfile");
Line 17: }
Line 18:
Line 19: @Override
Line 20: public VnicProfile get(Guid id) {
Why override?
Seems like the default behavior is fine (and this does exactly that)..
Line 21: return getCallsHandler().executeRead(getProcedureNameForGet(),
Line 22: VnicProfileRowMapper.INSTANCE,
Line 23: getCustomMapSqlParameterSource().addValue("id", id));
Line 24: }
Line 23: getCustomMapSqlParameterSource().addValue("id", id));
Line 24: }
Line 25:
Line 26: @Override
Line 27: public List<VnicProfile> getAll() {
Why override?
Seems like the default behavior is fine (and this does exactly that)..
Line 28: return
getCallsHandler().executeReadList(getProcedureNameForGetAll(),
Line 29: VnicProfileRowMapper.INSTANCE,
Line 30: getCustomMapSqlParameterSource());
Line 31: }
Line 38: }
Line 39:
Line 40: @Override
Line 41: protected MapSqlParameterSource
createFullParametersMapper(VnicProfile profile) {
Line 42: return getCustomMapSqlParameterSource()
You can simply call createIdParameterMapper(provile.getId())
No need to duplicate the name of "id" parameter
Line 43: .addValue("id", profile.getId())
Line 44: .addValue("name", profile.getName())
Line 45: .addValue("network_id", profile.getNetworkId())
Line 46: .addValue("port_mirroring", profile.isPortMirroring())
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/network/VnicProfileDaoTest.java
Line 13: import org.ovirt.engine.core.dao.BaseDAOTestCase;
Line 14: import org.ovirt.engine.core.dao.FixturesTool;
Line 15:
Line 16:
Line 17: public class VnicProfileDaoTest extends BaseDAOTestCase {
You can extend BaseGenericDaoTestCase which will save you some of the trouble
Line 18:
Line 19: private VnicProfile vnicProfile;
Line 20: private VnicProfileDao dao;
Line 21:
....................................................
File backend/manager/modules/dal/src/test/resources/fixtures.xml
Line 919: <value>fd81f1e1-785b-4579-ab75-1419ebb87052</value>
Line 920: <value>engine_profile</value>
Line 921: <value>58d5c1c6-cb15-4832-b2a4-023770607188</value>
Line 922: <value>false</value>
Line 923: <value></value>
Please put some values so this can be also tested..
Line 924: <value>vnic profile description</value>
Line 925: <value>2013-07-02 08:38:36</value>
Line 926: <value>2013-07-02 08:38:36</value>
Line 927: </row>
....................................................
File packaging/dbscripts/create_views.sql
Line 1523: FROM user_vnic_profile_permissions_view_base
Line 1524: NATURAL JOIN user_flat_groups;
Line 1525:
Line 1526: -- Permissions on Networks
Line 1527: -- The user has permissions on the Network directly
This comment is not relevant since you're not checking this
Line 1528: CREATE OR REPLACE VIEW user_network_permissions_view_base
(entity_id, granted_id)
Line 1529: AS
Line 1530: -- Or the user has permissions on one of the Network's VNIC Profiles
Line 1531: SELECT network.id, user_id
....................................................
File packaging/dbscripts/network_sp.sql
Line 1014: LANGUAGE plpgsql;
Line 1015:
Line 1016:
Line 1017:
----------------------------------------------------------------------
Line 1018: -- Vnic Profile
Please put all these new SPS in a separate file
Line 1019:
----------------------------------------------------------------------
Line 1020:
Line 1021: Create or replace FUNCTION GetVnicProfileByVnicProfileId(v_id UUID)
Line 1022: RETURNS SETOF vnic_profiles
Line 1059: BEGIN
Line 1060:
Line 1061: UPDATE vnic_profiles
Line 1062: SET id = v_id, name = v_name, network_id = v_network_id,
Line 1063: port_mirroring = v_port_mirroring, custom_properties =
v_custom_properties,
Please remove TWS
Line 1064: description = v_description,_update_date = LOCALTIMESTAMP
Line 1065: WHERE id = v_id;
Line 1066:
Line 1067: END; $procedure$
Line 1076: BEGIN
Line 1077:
Line 1078: -- Get (and keep) a shared lock with "right to upgrade to
exclusive"
Line 1079: -- in order to force locking parent before children
Line 1080: SELECT id INTO v_val FROM vnic_profiles WHERE id = v_id FOR
UPDATE;
Is this all really necessary?
AFAIK in postgresql we don't use this methodology, but just delete
Line 1081:
Line 1082: DELETE FROM vnic_profiles
Line 1083: WHERE id = v_id;
Line 1084:
....................................................
File packaging/dbscripts/upgrade/03_03_0620_add_profile_to_network_interface.sql
Line 7: name VARCHAR(50) NOT NULL,
Line 8: network_id UUID NOT NULL,
Line 9: port_mirroring BOOLEAN NOT NULL,
Line 10: custom_properties TEXT,
Line 11: description TEXT,
Should you also be adding a "comment" field (also in entity, of course)?
I don't know why, but it seems the norm these last days..
Line 12: _create_date TIMESTAMP WITH TIME ZONE default LOCALTIMESTAMP,
Line 13: _update_date TIMESTAMP WITH TIME ZONE,
Line 14: CONSTRAINT PK_vnic_id PRIMARY KEY (id)
Line 15: ) WITH OIDS;
Line 10: custom_properties TEXT,
Line 11: description TEXT,
Line 12: _create_date TIMESTAMP WITH TIME ZONE default LOCALTIMESTAMP,
Line 13: _update_date TIMESTAMP WITH TIME ZONE,
Line 14: CONSTRAINT PK_vnic_id PRIMARY KEY (id)
Shouldn't thie be called pk_vnic_profile_id?
Also I'm quite sure this can be inlined in the column definition row..
Line 15: ) WITH OIDS;
Line 16:
Line 17: ALTER TABLE vnic_profiles ADD CONSTRAINT FK_vnic_profiles_network_id
FOREIGN KEY(network_id)
Line 18: REFERENCES network(id) ON DELETE CASCADE;
Line 13: _update_date TIMESTAMP WITH TIME ZONE,
Line 14: CONSTRAINT PK_vnic_id PRIMARY KEY (id)
Line 15: ) WITH OIDS;
Line 16:
Line 17: ALTER TABLE vnic_profiles ADD CONSTRAINT FK_vnic_profiles_network_id
FOREIGN KEY(network_id)
I'm quite sure this can be inlined..
Line 18: REFERENCES network(id) ON DELETE CASCADE;
Line 19:
Line 20: DROP INDEX IF EXISTS IDX_vnic_profiles_network_id;
Line 21: CREATE INDEX IDX_vnic_profiles_network_id ON vnic_profiles(network_id);
Line 95: FROM vnic_profiles
Line 96: LEFT JOIN network ON network.id = vnic_profiles.network_id
Line 97: LEFT JOIN vds_groups ON network.storage_pool_id =
vds_groups.storage_pool_id
Line 98: LEFT JOIN vm_static ON vm_static.vds_group_id =
vds_groups.vds_group_id
Line 99: WHERE vm_interface.vmt_guid = vm_static.vm_guid AND
vm_interface.port_mirroring = vnic_profiles.port_mirroring;
Won't this override the values set in the previous update?
I don't see an exclusion here update only when vmt_guid is not null..
Line 100:
Line 101: -- drop the port_mirroring column from vm_interface
Line 102: SELECT fn_db_drop_column ('port_mirroring', 'vm_interface');
Line 103:
--
To view, visit http://gerrit.ovirt.org/15992
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id0525a6d30995fe896499fed283638b93cae5e41
Gerrit-PatchSet: 41
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: ofri masad <oma...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvall...@redhat.com>
Gerrit-Reviewer: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
