Moti Asayag has posted comments on this change. Change subject: core : Add Entity API for CoCo commands ......................................................................
Patch Set 6: (17 comments) > @Moti > > Should I rename JobSubjectEntityDaoFacadeImpl? to which name ? https://gerrit.ovirt.org/#/c/37463/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/interfaces/CommandScheduler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/tasks/interfaces/CommandScheduler.java: Line 13: public interface CommandScheduler { Line 14: Future<VdcReturnValueBase> executeAsyncCommand(VdcActionType actionType, Line 15: VdcActionParametersBase parameters, Line 16: CommandContext cmdContext); Line 17: Future<VdcReturnValueBase> executeAsyncCommand(VdcActionType actionType, why not to have only this api ? wouldn't it satisfies the requirement from it ? Future<VdcReturnValueBase> executeAsyncCommand(VdcActionType actionType, VdcActionParametersBase parameters, CommandContext cmdContext, SubjectEntity... entities) Line 18: VdcActionParametersBase parameters, Line 19: CommandContext cmdContext, Line 20: VdcObjectType entityType, Line 21: Guid... entityIds); https://gerrit.ovirt.org/#/c/37463/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/CommandAssociatedEntity.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/CommandAssociatedEntity.java: please implement toString() using ToStringBuilder introduced by mperina Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import org.ovirt.engine.core.common.VdcObjectType; Line 4: import org.ovirt.engine.core.common.utils.ObjectUtils; Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import org.ovirt.engine.core.common.VdcObjectType; Line 4: import org.ovirt.engine.core.common.utils.ObjectUtils; please use java.utils.Objects instead. Line 5: import org.ovirt.engine.core.compat.Guid; Line 6: Line 7: public class CommandAssociatedEntity extends SubjectEntity { Line 8: Line 3: import org.ovirt.engine.core.common.VdcObjectType; Line 4: import org.ovirt.engine.core.common.utils.ObjectUtils; Line 5: import org.ovirt.engine.core.compat.Guid; Line 6: Line 7: public class CommandAssociatedEntity extends SubjectEntity { for the sake of consistency I'd call this CommandSubjectEntity Line 8: Line 9: private Guid commandId; Line 10: Line 11: public CommandAssociatedEntity() { Line 11: public CommandAssociatedEntity() { Line 12: } Line 13: Line 14: public CommandAssociatedEntity(Guid commandId, VdcObjectType entityType, Guid entityId) { Line 15: this.commandId = commandId; this c'tor implies the entity is immutable. Please let have same assumption for SubjectEntity class and introduce a c'tor there which expect both entityType and entityId and use it here instead of the setters. Line 16: setEntityType(entityType); Line 17: setEntityId(entityId); Line 18: } Line 19: Line 20: @Override Line 21: public int hashCode() { Line 22: final int prime = 31; Line 23: int result = super.hashCode(); Line 24: result = prime * result + ((commandId == null) ? 0 : commandId.hashCode()); this can be simplified by using Objects.hashCode(commandId) Line 25: return result; Line 26: } Line 27: Line 28: @Override https://gerrit.ovirt.org/#/c/37463/6/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/SubjectEntity.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/SubjectEntity.java: please implement toString() using ToStringBuilder introduced by mperina Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import org.ovirt.engine.core.common.VdcObjectType; Line 4: import org.ovirt.engine.core.common.utils.ObjectUtils; Line 1: package org.ovirt.engine.core.common.businessentities; Line 2: Line 3: import org.ovirt.engine.core.common.VdcObjectType; Line 4: import org.ovirt.engine.core.common.utils.ObjectUtils; please use java.utils.Objects instead. Line 5: import org.ovirt.engine.core.compat.Guid; Line 6: Line 7: public class SubjectEntity { Line 8: private Guid entityId; Line 3: import org.ovirt.engine.core.common.VdcObjectType; Line 4: import org.ovirt.engine.core.common.utils.ObjectUtils; Line 5: import org.ovirt.engine.core.compat.Guid; Line 6: Line 7: public class SubjectEntity { since this entity is exactly as JobSubjectEntity, it would make sense to replace JobSubjectEntity with this one. can be done in the future and not as part of this patch. Line 8: private Guid entityId; Line 9: private VdcObjectType entityType; Line 10: Line 11: public void setEntityId(Guid entityId) { Line 28: public int hashCode() { Line 29: final int prime = 31; Line 30: int result = 1; Line 31: result = prime * result Line 32: + ((entityId == null) ? 0 : entityId.hashCode()); as in previous file - consider using Objects.hashCode() Line 33: result = prime * result Line 34: + ((entityType == null) ? 0 : entityType.hashCode()); Line 35: return result; Line 36: } Line 42: } Line 43: if (obj == null) { Line 44: return false; Line 45: } Line 46: if (getClass() != obj.getClass()) { this is a bit problematic, since if we compare SubjectEntity to CommandAssociatedEntity, even if they represent the same entity, this will fail due to different class types. Please consider using instanceof instead of class comparison. Line 47: return false; Line 48: } Line 49: SubjectEntity other = (SubjectEntity) obj; Line 50: return ObjectUtils.objectsEqual(entityId, other.entityId) && https://gerrit.ovirt.org/#/c/37463/6/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/CommandEntityDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/CommandEntityDaoDbFacadeImpl.java: Line 203: return getCallsHandler().executeReadList("GetCommandIdsByEntityId", IdRowMapper.instance, parameterSource); Line 204: } Line 205: Line 206: public void insertCommandAssociatedEntities(Collection<CommandAssociatedEntity> cmdAssociatedEntities) { Line 207: getCallsHandler().executeStoredProcAsBatch("InsertCommandAssociatedEntities", cmdAssociatedEntities, cocoCmdEntityMapper); please format (line too long) Line 208: } Line 209: Line 210: @Override Line 211: public List<CommandAssociatedEntity> getAllCommandAssociatedEntities(Guid cmdId) { https://gerrit.ovirt.org/#/c/37463/6/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/CommandEntityDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/CommandEntityDaoTest.java: Line 94: dao.insertCommandAssociatedEntities(cocoCmdEntities2); Line 95: Line 96: List<Guid> cmIds = dao.getCommandIdsByEntity(storageId); Line 97: assertNotNull(cmIds); Line 98: assertEquals(2, cmIds.size()); can be replaced with: assertThat(cmdIds, hasSize(2)); Line 99: assertTrue(cmIds.contains(cmdEntity1.getId())); Line 100: assertTrue(cmIds.contains(cmdEntity2.getId())); Line 101: } Line 102: Line 96: List<Guid> cmIds = dao.getCommandIdsByEntity(storageId); Line 97: assertNotNull(cmIds); Line 98: assertEquals(2, cmIds.size()); Line 99: assertTrue(cmIds.contains(cmdEntity1.getId())); Line 100: assertTrue(cmIds.contains(cmdEntity2.getId())); please replace the two above with this assertion: assertThat(cmIds, hasItems(cmdEntity1.getId(), cmdEntity2.getId())); where hasItems is a matcher taken retrieved by: import static org.hamcrest.Matchers.hasItems Line 101: } Line 102: Line 103: @Test Line 104: public void testInsertAsyncTaskEntitities() { Line 100: assertTrue(cmIds.contains(cmdEntity2.getId())); Line 101: } Line 102: Line 103: @Test Line 104: public void testInsertAsyncTaskEntitities() { seems that testGetAllCommandAssociatedEntities is a better match for the name Line 105: CommandEntity cmdEntity = generateNewEntity(); Line 106: dao.save(cmdEntity); Line 107: Set<CommandAssociatedEntity> cocoCmdEntities = new HashSet<>(); Line 108: cocoCmdEntities.add(new CommandAssociatedEntity(cmdEntity.getId(), VdcObjectType.Storage, Guid.newGuid())); Line 111: List<CommandAssociatedEntity> entities = dao.getAllCommandAssociatedEntities(cmdEntity.getId()); Line 112: assertNotNull(entities); Line 113: assertEquals(2, entities.size()); Line 114: for (CommandAssociatedEntity entity : cocoCmdEntities) { Line 115: assertTrue(entities.contains(entity)); same as above. Line 116: } Line 117: Line 118: } https://gerrit.ovirt.org/#/c/37463/6/packaging/dbscripts/upgrade/03_06_1320_add_cmd_associated_entities_table.sql File packaging/dbscripts/upgrade/03_06_1320_add_cmd_associated_entities_table.sql: Line 1: command_assoc_entities in case you agree to the command about the renaming of CommandAssociatedEntity to CommandSubjectEntity, i think this table should be renamed to match the entity name. -- To view, visit https://gerrit.ovirt.org/37463 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I96b934ffc44f87407df77c8e097164ca30f3a6e6 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <wallaroo1...@gmail.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
