Shireesh Anjal has posted comments on this change.
Change subject: engine: adding vds_gluster table and dao operations
......................................................................
Patch Set 5: (13 inline comments)
....................................................
File backend/manager/dbscripts/upgrade/03_03_0060_add_vds_gluster_table.sql
Line 1: Create or replace FUNCTION fn_add_vds_gluster_table()
Line 2: RETURNS void
Line 3: AS $function$
Line 4: BEGIN
Line 5: CREATE TABLE vds_gluster
I know that you would have named this table as vds_gluster to keep it
consistent with other tables like vds_static, vds_dynamic, etc. But since the
terms VDS or host are not appropriate in gluster context, and we've been using
the term "server", I suggest to change the table name to gluster_server, and
the column names to server_id and gluster_server_uuid.
And similar changes in the Java business entity class as well.
Line 6: (
Line 7: vds_id UUID NOT NULL references vds_static(vds_id) ON DELETE
CASCADE,
Line 8: gluster_host_uuid UUID NOT NULL,
Line 9: CONSTRAINT pk_vds_gluster PRIMARY KEY(vds_id)
Line 7: vds_id UUID NOT NULL references vds_static(vds_id) ON DELETE
CASCADE,
Line 8: gluster_host_uuid UUID NOT NULL,
Line 9: CONSTRAINT pk_vds_gluster PRIMARY KEY(vds_id)
Line 10: ) WITH OIDS;
Line 11: CREATE INDEX IDX_vds_gluster_vds_id ON vds_gluster(vds_id);
Since vds_id is the primary key, I don't think you need to create another index
on it.
Line 12: CREATE UNIQUE INDEX IDX_vds_gluster_unique ON vds_gluster(vds_id,
gluster_host_uuid);
Line 13: END; $function$
Line 14: LANGUAGE plpgsql;
Line 15:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/VdsGluster.java
Line 7: public class VdsGluster implements BusinessEntity<Guid> {
Line 8:
Line 9: private static final long serialVersionUID = -1425566208615075937L;
Line 10:
Line 11: private Guid id;
I'd prefer this to be called vdsId
Line 12:
Line 13: private Guid glusterHostUuid;
Line 14:
Line 15: @Override
Line 46: }
Line 47:
Line 48: VdsGluster entity = (VdsGluster) obj;
Line 49:
Line 50: if (!(ObjectUtils.objectsEqual(getId(), entity.getId())
How about:
return ObjectUtils.objectsEqual(getId(), entity.getId()) &&
ObjectUtils.objectsEqual(glusterHostUuid, entity.getGlusterHostUuid());
?
Also, use either the field or getter in both conditions, instead of getter for
id and field for glusterHostUuid.
Line 51: && ObjectUtils.objectsEqual(glusterHostUuid,
entity.getGlusterHostUuid()))) {
Line 52: return false;
Line 53: }
Line 54: return true;
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/VdsGlusterDaoDbFacadeImpl.java
Line 18: setProcedureNameForGet("GetVdsGlusterByVdsId");
Line 19: }
Line 20:
Line 21: @Override
Line 22: public void save(VdsGluster vdsGluster) {
No need to implement this method as your code is exactly same as what
DefaultGenericDaoDbFacade#save() does.
Line 23: getCallsHandler().executeModification("InsertVdsGluster",
createFullParametersMapper(vdsGluster));
Line 24: }
Line 25:
Line 26: @Override
Line 36: }
Line 37:
Line 38: @Override
Line 39: public void update(Guid vdsId, Guid glusterHostUuid) {
Line 40: getCallsHandler().executeModification("UpdateVdsGluster",
This method also need not be implemented, as the one defined in
DefaultGenericDaoDbFacade does exactly the same!
Line 41: getCustomMapSqlParameterSource()
Line 42: .addValue("vds_id", vdsId)
Line 43: .addValue("gluster_host_uuid",
glusterHostUuid));
Line 44: }
Line 45:
Line 46: @Override
Line 47: public void remove(Guid id) {
Line 48:
getCallsHandler().executeModification("DeleteVdsGlusterByVdsId",
Line 49: createIdParameterMapper(id));
If you rename the SP to just "DeleteVdsGluster", this method also need not be
implemented.
Line 50: }
Line 51:
Line 52: @Override
Line 53: public void removeByGlusterHostUuid(Guid glusterHostUuid) {
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/VdsGlusterDao.java
Line 8: public interface VdsGlusterDao extends DAO,
MassOperationsDao<VdsGluster, Guid> {
Line 9:
Line 10: public void save(VdsGluster vdsGluster);
Line 11:
Line 12: public VdsGluster getById(Guid id);
I'd prefer this to be called
getByVdsId(Guid vdsId);
Line 13:
Line 14: public VdsGluster getByGlusterHostUuid(Guid glusterHostUuid);
Line 15:
Line 16: public void remove(Guid id);
Line 12: public VdsGluster getById(Guid id);
Line 13:
Line 14: public VdsGluster getByGlusterHostUuid(Guid glusterHostUuid);
Line 15:
Line 16: public void remove(Guid id);
I'd prefer this to be called removeByVdsid(Guid vdsId);
Line 17:
Line 18: public void removeByGlusterHostUuid(Guid glusterHostUuid);
Line 19:
Line 20: public void update(Guid vdsId, Guid glusterHostUuid);
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/gluster/VdsGlusterDaoTest.java
Line 32: newEntity.setGlusterHostUuid(FixturesTool.GLUSTER_HOST_UUID2);
Line 33:
Line 34: dao.save(newEntity);
Line 35: VdsGluster entity = dao.getById(newEntity.getId());
Line 36: assertTrue(newEntity.equals(entity));
minor: Maybe assertEquals is more suited here :)
Line 37: }
Line 38:
Line 39: @Test
Line 40: public void testGetById() {
Line 39: @Test
Line 40: public void testGetById() {
Line 41: VdsGluster entity = dao.getById(SERVER_ID1);
Line 42: assertNotNull(entity);
Line 43: assertEquals(SERVER_ID1, entity.getId());
I think you should compare the other values (gluster uuid) as well
Line 44: }
Line 45:
Line 46: @Test
Line 47: public void testGetByGlusterHostUuid() {
Line 46: @Test
Line 47: public void testGetByGlusterHostUuid() {
Line 48: VdsGluster entity =
dao.getByGlusterHostUuid(FixturesTool.GLUSTER_HOST_UUID1);
Line 49: assertNotNull(entity);
Line 50: assertEquals(entity.getGlusterHostUuid(),
FixturesTool.GLUSTER_HOST_UUID1);
Same here. I think you should compare the server id also.
Line 51: }
Line 52:
Line 53: @Test
Line 54: public void testRemove() {
Line 68: public void testUpdateGlusterHostUuid() {
Line 69: dao.update(SERVER_ID1, FixturesTool.GLUSTER_HOST_UUID_NEW);
Line 70: VdsGluster entity = dao.getById(SERVER_ID1);
Line 71: assertNotNull(entity);
Line 72: assertEquals(entity.getGlusterHostUuid(),
FixturesTool.GLUSTER_HOST_UUID_NEW);
I believe expected value (FixturesTool.GLUSTER_HOST_UUID_NEW) should be the
first argument to assertEquals, and actual one (entity.getGlusterHostUuid())
the second.
Line 73: }
--
To view, visit http://gerrit.ovirt.org/14063
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I04ba0a7bf1eaa964db731cdfa24ab6875e0b1513
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Eli Mesika <emes...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
