Moti Asayag has posted comments on this change. Change subject: core: Move Job, Step to JPA ......................................................................
Patch Set 51: (5 comments) https://gerrit.ovirt.org/#/c/34553/51/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 47: @Type(type = "org.ovirt.engine.core.dao.jpa.GuidUserType") Line 48: private Guid jobId; Line 49: Line 50: public JobSubjectEntity() { Line 51: super(); please remove empty c'tor Line 52: } Line 53: Line 54: public JobSubjectEntity(Guid jobId, Guid entityId, VdcObjectType entityType) { Line 55: super(); Line 51: super(); Line 52: } Line 53: Line 54: public JobSubjectEntity(Guid jobId, Guid entityId, VdcObjectType entityType) { Line 55: super(); please remove empty c'tor Line 56: this.entityId = entityId; Line 57: this.entityType = entityType; Line 58: this.jobId = jobId; Line 59: } https://gerrit.ovirt.org/#/c/34553/51/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 33: Line 34: @Override Line 35: public boolean equals(Object obj) { Line 36: if (this == obj) Line 37: return true; repeated twice Line 38: if (this == obj) { Line 39: return true; Line 40: } Line 41: if (!(obj instanceof JobSubjectEntityId)) { https://gerrit.ovirt.org/#/c/34553/51/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 42: allJobs.addAll(multipleResults(entityManager.createNamedQuery("Job.getJobsByOffsetAndPageSizeNotInStatus", Line 43: Job.class) Line 44: .setParameter("status", EnumSet.of(JobExecutionStatus.STARTED, Line 45: JobExecutionStatus.UNKNOWN)))); Line 46: 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 47: return allJobs.subList(offset, endIndex); Line 48: } Line 49: Line 50: @Override https://gerrit.ovirt.org/#/c/34553/51/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.updateJobStepsCompleted") 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: 51 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
