Moti Asayag has posted comments on this change. Change subject: core: Move Job, Step to JPA ......................................................................
Patch Set 54: (5 comments) before diving into the specific comments for this patch, i'd like to reconsider for a second the thought of having the NamedQuery as part of the entity. The entity itself doesn't seem like the natural place for storing the named query. I think we'll have to debate between ourself when there will be cross-entities queries about the correct location for those entities. In addition, it makes the entity file too big, too detailed and presume the entity itself should know about the data layer (which is only partially true due to hibernate-annotations). We can spare the named query from the business entities and having them in more natural place - on the DAOs themselves [1] What's your opinion ? [1] http://stackoverflow.com/a/8740881 https://gerrit.ovirt.org/#/c/34553/54/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 54: + "or (j.endTime < :successEndTime and j.status = :successStatus))"), Line 55: @NamedQuery( Line 56: name = "Job.deleteJobOlderThanDateWithStatus", Line 57: query = "delete from Job j where j.autoCleared = true and j.endTime < :sinceDate " Line 58: + "and j.status in (:statuses)") please replace tabs with spaces. Line 59: }) Line 60: public class Job extends IVdcQueryable implements BusinessEntity<Guid> { Line 61: Line 62: /** https://gerrit.ovirt.org/#/c/34553/54/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: please replaces tabs with spaces. Line 1: package org.ovirt.engine.core.common.job; Line 2: Line 3: import java.util.ArrayList; Line 4: import java.util.Date; Line 35: @Cacheable(true) Line 36: @NamedQueries({ Line 37: @NamedQuery( Line 38: name = "Step.updateJobStepsCompleted", Line 39: query = "update Step s set s.status = :status, s.endTime = :endTime " i'd rather more distinguishable format: update Step s set s.status = :status, s.endTime = :endTime where s.status = :startedStatus and s.jobId = :jobId and if where clause became too long, to break it to condition per line. Line 40: + "where s.status = :startedStatus and s.jobId = :jobId"), Line 41: @NamedQuery( Line 42: name = "Step.getStepsByExternalId", Line 43: query = "select s from Step s where s.externalSystem.externalId = :externalId " Line 39: query = "update Step s set s.status = :status, s.endTime = :endTime " Line 40: + "where s.status = :startedStatus and s.jobId = :jobId"), Line 41: @NamedQuery( Line 42: name = "Step.getStepsByExternalId", Line 43: query = "select s from Step s where s.externalSystem.externalId = :externalId " here: select s from Step s where s.externalSystem.externalId = :externalId order by s.parentStepId, s.stepNumber also please have the sql lines aligned to the same offset. Line 44: + "order by s.parentStepId, s.stepNumber"), Line 45: @NamedQuery( Line 46: name = "Step.getExternalIdsForRunningSteps", Line 47: query = "select s.externalSystem.externalId from Job j inner join j.steps s " Line 41: @NamedQuery( Line 42: name = "Step.getStepsByExternalId", Line 43: query = "select s from Step s where s.externalSystem.externalId = :externalId " Line 44: + "order by s.parentStepId, s.stepNumber"), Line 45: @NamedQuery( same for the two below. Line 46: name = "Step.getExternalIdsForRunningSteps", Line 47: query = "select s.externalSystem.externalId from Job j inner join j.steps s " Line 48: + "where j.status = :status and s.externalSystem.externalSystemType = :type"), Line 49: @NamedQuery( -- 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: 54 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
