Martin Peřina has posted comments on this change. Change subject: core: Move Job, Step to JPA ......................................................................
Patch Set 49: (7 comments) Btw, shouldn't we use only primary keys inside hashCode() and equals() for JPA based entities? https://gerrit.ovirt.org/#/c/34553/49/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 51: query = "delete from Job j where j.autoCleared = true and ((j.endTime < :failedEndTime and j.status in (:failStatus)) or (j.endTime < :successEndTime and j.status = :successStatus))"), Line 52: @NamedQuery( Line 53: name = "Job.deleteJobOlderThanDateWithStatus", Line 54: query = "delete from Job j where j.autoCleared = true and j.endTime < :sinceDate and j.status in (:statuses)") Line 55: }) It would be nice to have some naming convention for NamedQueries, so I would prefer the query name to start with lower case: Job.byCorrelationId Job.getJobsInStatus Job.getJobsNotInStatus Job.deleteCompletedJobs Job.deleteJobOlderThanDateWithStatus Line 56: public class Job extends IVdcQueryable implements BusinessEntity<Guid> { Line 57: Line 58: /** Line 59: * Automatic generated serial version ID https://gerrit.ovirt.org/#/c/34553/49/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 94: jobId = id.getJobId(); Line 95: } Line 96: Line 97: @Override Line 98: public int hashCode() { Why not use Objects.hash()? return Objects.hash(entityId, entityType, jobId); Line 99: final int prime = 31; 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 115: if (getClass() != obj.getClass()) { : return false; : } I would prefer to use smallest possible equals() as proposed in Effective Java: @Override public boolean equals(Object obj) { if (this == obj) { return true; } if (!(obj instanceof JobSubjectEntity)) { return false; } JobSubjectEntity other = (JobSubjectEntity) obj; ... } Line 132: } Line 133: } else if (!jobId.equals(other.jobId)) { Line 134: return false; Line 135: } Line 136: return true; Why not use Objects.equals()? return Objects.equals(entityId, other.entityId) && Objects.equals(jobId, other.jobId); Line 137: } https://gerrit.ovirt.org/#/c/34553/49/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 25: this.jobId = jobId; Line 26: } Line 27: Line 28: @Override Line 29: public int hashCode() { return Objects.hash(entityId, jobId); Line 30: final int prime = 31; Line 31: int result = 1; Line 32: result = prime * result + ((entityId == null) ? 0 : entityId.hashCode()); Line 33: result = prime * result + ((jobId == null) ? 0 : jobId.hashCode()); Line 34: return result; Line 35: } Line 36: Line 37: @Override Line 38: public boolean equals(Object obj) { @Override public boolean equals(Object obj) { if (this == obj) { return true; } if (!(obj instanceof JobSubjectEntityId)) { return false; } JobSubjectEntityId other = (JobSubjectEntityId) obj; return Objects.equals(entityId, other.entityId) && Objects.equals(jobId, other.jobId); } Line 39: if (this == obj) Line 40: return true; Line 41: if (obj == null) Line 42: return false; https://gerrit.ovirt.org/#/c/34553/49/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 91: @Override Line 92: public boolean checkIfJobHasTasks(Guid jobId) { Line 93: List<Step> steps = step.getStepsByJobIdForVdsmAndGluster(jobId); Line 94: Line 95: return steps.size() > 0; Please use: return !steps.isEmpty(); Line 96: } -- 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: 49 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: Martin Peřina <mper...@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
