Moti Asayag has posted comments on this change. Change subject: core: Move Job, Step to JPA ......................................................................
Patch Set 48: (11 comments) https://gerrit.ovirt.org/#/c/34553/48/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/Job.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/Job.java: Line 28: import org.ovirt.engine.core.common.VdcObjectType; Line 29: import org.ovirt.engine.core.common.action.VdcActionType; Line 30: import org.ovirt.engine.core.common.businessentities.BusinessEntity; Line 31: import org.ovirt.engine.core.common.businessentities.IVdcQueryable; Line 32: import org.ovirt.engine.core.common.utils.ObjectUtils; please use Line 33: import org.ovirt.engine.core.compat.Guid; Line 34: Line 35: /** Line 36: * Represents the Job entity which encapsulates a client action in the system. The Job contains a collection Line 375: result = prime * result + (autoCleared ? 1231 : 1237); Line 376: return result; Line 377: } Line 378: Line 379: @Override i'd leave here on the job-id. Line 380: public boolean equals(Object obj) { Line 381: if (this == obj) { Line 382: return true; Line 383: } https://gerrit.ovirt.org/#/c/34553/48/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/JobSubjectEntity.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/JobSubjectEntity.java: Line 45: @Type(type = "org.ovirt.engine.core.dao.jpa.GuidUserType") Line 46: private Guid jobId; Line 47: Line 48: public JobSubjectEntity() { Line 49: super(); please remove super(); Line 50: } Line 51: Line 52: public JobSubjectEntity(Guid jobId, Guid entityId, VdcObjectType entityType) { Line 53: super(); Line 49: super(); Line 50: } Line 51: Line 52: public JobSubjectEntity(Guid jobId, Guid entityId, VdcObjectType entityType) { Line 53: super(); please remove super(); Line 54: this.entityId = entityId; Line 55: this.entityType = entityType; Line 56: this.jobId = jobId; Line 57: } Line 95: } Line 96: Line 97: @Override Line 98: public int hashCode() { Line 99: final int prime = 31; please replace the implementation with java 7 java.util.Objects: return Objects.hash(ehtityId, jobId); Line 100: int result = 1; Line 101: result = prime * result + ((entityId == null) ? 0 : entityId.hashCode()); Line 102: result = prime * result + ((entityType == null) ? 0 : entityType.hashCode()); Line 103: result = prime * result + ((jobId == null) ? 0 : jobId.hashCode()); Line 104: return result; Line 105: } Line 106: Line 107: @Override Line 108: public boolean equals(Object obj) { please implement using java 7 Objects.equals() which simplifies the method. we should have here only the business entity id fields (i.e. jobId and entityId). Line 109: if (this == obj) { Line 110: return true; Line 111: } Line 112: if (obj == null) { https://gerrit.ovirt.org/#/c/34553/48/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/JobSubjectEntityId.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/JobSubjectEntityId.java: Line 24: public void setJobId(Guid jobId) { Line 25: this.jobId = jobId; Line 26: } Line 27: Line 28: @Override same comments as previous file. Line 29: public int hashCode() { Line 30: final int prime = 31; Line 31: int result = 1; Line 32: result = prime * result + ((entityId == null) ? 0 : entityId.hashCode()); Line 34: return result; Line 35: } Line 36: Line 37: @Override Line 38: public boolean equals(Object obj) { same Line 39: if (this == obj) Line 40: return true; Line 41: if (obj == null) Line 42: return false; https://gerrit.ovirt.org/#/c/34553/48/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/Step.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/Step.java: Line 354: } Line 355: Line 356: @Override Line 357: public boolean equals(Object obj) { Line 358: if (this == obj) { it should be updated to contain only the step-id as a candidate for equality. Line 359: return true; Line 360: } Line 361: if (obj == null) { Line 362: return false; https://gerrit.ovirt.org/#/c/34553/48/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/JobDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/JobDaoDbFacadeImpl.java: Line 40: JobExecutionStatus.STARTED)); Line 41: allJobs.addAll(multipleResults(entityManager.createNamedQuery("Job.GetJobsNotInStatus", Job.class) Line 42: .setParameter("status", EnumSet.of(JobExecutionStatus.STARTED, Line 43: JobExecutionStatus.UNKNOWN)))); Line 44: int endIndex = Math.min(offset + pageSize, allJobs.size()); how would this code behave ? those entities are cache-able, but if we happen to have a large amount of those entities, wouldn't all of them have to be fetched first and only later to select the relevant portion ? Would it be preferable to delegate the relevant section to the query ? Line 45: return allJobs.subList(offset, endIndex); Line 46: } Line 47: Line 48: @Override https://gerrit.ovirt.org/#/c/34553/48/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StepDaoDbFacadeImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StepDaoDbFacadeImpl.java: Line 44: } Line 45: Line 46: @Override Line 47: public void updateJobStepsCompleted(Guid jobId, JobExecutionStatus status, Date endTime) { Line 48: if (status != JobExecutionStatus.STARTED) { this introduces business logic into the data layer. Line 49: updateQuery(entityManager.createNamedQuery("Step.updateStatusEndTime") Line 50: .setParameter("jobId", jobId) Line 51: .setParameter("status", status) Line 52: .setParameter("endTime", endTime) -- To view, visit https://gerrit.ovirt.org/34553 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifcfda7b055d37c92c1346b100101c27d594d21fb Gerrit-PatchSet: 48 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liran Zelkha <lzel...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@redhat.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
