diff --git a/genie-web/src/integTest/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsIntegrationTest.java b/genie-web/src/integTest/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsIntegrationTest.java index e8b6332a747..d04341b5ada 100644 --- a/genie-web/src/integTest/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsIntegrationTest.java +++ b/genie-web/src/integTest/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsIntegrationTest.java @@ -422,20 +422,15 @@ void canUpdateJobStatus() throws GenieCheckedException, IOException { Assertions.assertThat(jobEntity.getStatus()).isEqualTo(JobStatus.CLAIMED.name()); - // status won't match so it will throw exception + // status won't match so it won't update Assertions - .assertThatExceptionOfType(GenieInvalidStatusException.class) - .isThrownBy( - () -> this.service.updateJobStatus( - jobId, - JobStatus.RUNNING, - JobStatus.FAILED, - null - ) - ); + .assertThat(this.service.updateJobStatus(jobId, JobStatus.RUNNING, JobStatus.FAILED, null)) + .isEqualTo(JobStatus.CLAIMED); final String initStatusMessage = "Job is initializing"; - this.service.updateJobStatus(jobId, JobStatus.CLAIMED, JobStatus.INIT, initStatusMessage); + Assertions + .assertThat(this.service.updateJobStatus(jobId, JobStatus.CLAIMED, JobStatus.INIT, initStatusMessage)) + .isEqualTo(JobStatus.INIT); jobEntity = this.jobRepository .findByUniqueId(jobId) @@ -448,7 +443,9 @@ void canUpdateJobStatus() throws GenieCheckedException, IOException { Assertions.assertThat(jobEntity.getFinished()).isNotPresent(); final String runningStatusMessage = "Job is running"; - this.service.updateJobStatus(jobId, JobStatus.INIT, JobStatus.RUNNING, runningStatusMessage); + Assertions + .assertThat(this.service.updateJobStatus(jobId, JobStatus.INIT, JobStatus.RUNNING, runningStatusMessage)) + .isEqualTo(JobStatus.RUNNING); jobEntity = this.jobRepository .findByUniqueId(jobId) @@ -459,15 +456,17 @@ void canUpdateJobStatus() throws GenieCheckedException, IOException { Assertions.assertThat(jobEntity.getStarted()).isPresent(); Assertions.assertThat(jobEntity.getFinished()).isNotPresent(); - final String successStatusMessage = "Job completed successfully"; - this.service.updateJobStatus(jobId, JobStatus.RUNNING, JobStatus.SUCCEEDED, successStatusMessage); + final String successMessage = "Job completed successfully"; + Assertions + .assertThat(this.service.updateJobStatus(jobId, JobStatus.RUNNING, JobStatus.SUCCEEDED, successMessage)) + .isEqualTo(JobStatus.SUCCEEDED); jobEntity = this.jobRepository .findByUniqueId(jobId) .orElseThrow(IllegalArgumentException::new); Assertions.assertThat(jobEntity.getStatus()).isEqualTo(JobStatus.SUCCEEDED.name()); - Assertions.assertThat(jobEntity.getStatusMsg()).isPresent().contains(successStatusMessage); + Assertions.assertThat(jobEntity.getStatusMsg()).isPresent().contains(successMessage); Assertions.assertThat(jobEntity.getStarted()).isPresent(); Assertions.assertThat(jobEntity.getFinished()).isPresent(); } diff --git a/genie-web/src/main/java/com/netflix/genie/web/data/services/PersistenceService.java b/genie-web/src/main/java/com/netflix/genie/web/data/services/PersistenceService.java index b1a4e3492a6..ce033ccc119 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/data/services/PersistenceService.java +++ b/genie-web/src/main/java/com/netflix/genie/web/data/services/PersistenceService.java @@ -695,23 +695,22 @@ void claimJob( * of the job matches {@code newStatus}. Optionally a status message can be provided to provide more details to * users. If the {@code newStatus} is {@link JobStatus#RUNNING} the start time will be set. If the {@code newStatus} * is a member of {@link JobStatus#getFinishedStatuses()} and the job had a started time set the finished time of - * the job will be set. + * the job will be set. If the {@literal currentStatus} is different from what the source of truth thinks this + * function will skip the update and just return the current source of truth value. * * @param id The id of the job to update status for. Must exist in the system. * @param currentStatus The status the caller to this API thinks the job currently has * @param newStatus The new status the caller would like to update the status to * @param newStatusMessage An optional status message to associate with this change - * @throws NotFoundException if no job with the given {@code id} exists - * @throws GenieInvalidStatusException if the current status of the job identified by {@code id} in the system - * doesn't match the supplied {@code currentStatus}. - * Also if the {@code currentStatus} equals the {@code newStatus}. + * @return The job status in the source of truth + * @throws NotFoundException if no job with the given {@code id} exists */ - void updateJobStatus( + JobStatus updateJobStatus( @NotBlank String id, @NotNull JobStatus currentStatus, @NotNull JobStatus newStatus, @Nullable String newStatusMessage - ) throws NotFoundException, GenieInvalidStatusException; + ) throws NotFoundException; /** * Update the status and status message of the job. diff --git a/genie-web/src/main/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImpl.java b/genie-web/src/main/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImpl.java index 5a3f27f7fbb..e9e03085282 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImpl.java +++ b/genie-web/src/main/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImpl.java @@ -1765,12 +1765,12 @@ public void claimJob( * {@inheritDoc} */ @Override - public void updateJobStatus( + public JobStatus updateJobStatus( @NotBlank final String id, @NotNull final JobStatus currentStatus, @NotNull final JobStatus newStatus, @Nullable final String newStatusMessage - ) throws NotFoundException, GenieInvalidStatusException { + ) throws NotFoundException { log.debug( "[updateJobStatus] Requested to change the status of job {} from {} to {} with message {}", id, @@ -1779,41 +1779,61 @@ public void updateJobStatus( newStatusMessage ); if (currentStatus == newStatus) { - throw new GenieInvalidStatusException( - "Can't update the status of job " + id + " because both current and new status are " + currentStatus + log.debug( + "[updateJobStatus] Requested new status for {} is same as current status: {}. Skipping update.", + id, + currentStatus ); + return newStatus; } final JobEntity jobEntity = this.getJobEntity(id); final JobStatus actualCurrentStatus = DtoConverters.toV4JobStatus(jobEntity.getStatus()); if (actualCurrentStatus != currentStatus) { - throw new GenieInvalidStatusException( - "Job " - + id - + " current status is " - + actualCurrentStatus - + " but API caller expected it to be " - + currentStatus - + ". Unable to update status due to inconsistent state." + log.warn( + "[updateJobStatus] Job {} actual status {} differs from expected status {}. Skipping update.", + id, + actualCurrentStatus, + currentStatus ); + return actualCurrentStatus; } - // TODO: Should we throw an exception if the job is already in a terminal state and someone is trying to - // further update it? In the private method below used in Genie 3 it's just swallowed and is a no-op - // TODO: Should we prevent updating status for statuses already covered by "reserveJobId" and // "saveResolvedJob"? - this.updateJobStatus(jobEntity, newStatus, newStatusMessage); + // Only change the status if the entity isn't already in a terminal state + if (actualCurrentStatus.isActive()) { + jobEntity.setStatus(newStatus.name()); + jobEntity.setStatusMsg(StringUtils.truncate(newStatusMessage, MAX_STATUS_MESSAGE_LENGTH)); - log.debug( - "[updateJobStatus] Changed the status of job {} from {} to {} with message {}", - id, - currentStatus, - newStatus, - newStatusMessage - ); + if (newStatus.equals(JobStatus.RUNNING)) { + // Status being changed to running so set start date. + jobEntity.setStarted(Instant.now()); + } else if (jobEntity.getStarted().isPresent() && newStatus.isFinished()) { + // Since start date is set the job was running previously and now has finished + // with status killed, failed or succeeded. So we set the job finish time. + jobEntity.setFinished(Instant.now()); + } + + log.debug( + "[updateJobStatus] Changed the status of job {} from {} to {} with message {}", + id, + currentStatus, + newStatus, + newStatusMessage + ); + + return newStatus; + } else { + log.warn( + "[updateJobStatus] Job status for {} is already terminal state {}. Skipping update.", + id, + actualCurrentStatus + ); + return actualCurrentStatus; + } } /** @@ -2615,28 +2635,6 @@ private void updateClusterCriteria(final CommandEntity commandEntity, final List ); } - private void updateJobStatus( - final JobEntity jobEntity, - final JobStatus newStatus, - @Nullable final String statusMsg - ) { - final JobStatus currentStatus = DtoConverters.toV4JobStatus(jobEntity.getStatus()); - // Only change the status if the entity isn't already in a terminal state - if (currentStatus.isActive()) { - jobEntity.setStatus(newStatus.name()); - jobEntity.setStatusMsg(StringUtils.truncate(statusMsg, MAX_STATUS_MESSAGE_LENGTH)); - - if (newStatus.equals(JobStatus.RUNNING)) { - // Status being changed to running so set start date. - jobEntity.setStarted(Instant.now()); - } else if (jobEntity.getStarted().isPresent() && newStatus.isFinished()) { - // Since start date is set the job was running previously and now has finished - // with status killed, failed or succeeded. So we set the job finish time. - jobEntity.setFinished(Instant.now()); - } - } - } - private void setJobMetadataFields( final JobEntity jobEntity, final JobMetadata jobMetadata, diff --git a/genie-web/src/main/java/com/netflix/genie/web/services/impl/JobLaunchServiceImpl.java b/genie-web/src/main/java/com/netflix/genie/web/services/impl/JobLaunchServiceImpl.java index 9aa3a9daefc..76543cb200d 100644 --- a/genie-web/src/main/java/com/netflix/genie/web/services/impl/JobLaunchServiceImpl.java +++ b/genie-web/src/main/java/com/netflix/genie/web/services/impl/JobLaunchServiceImpl.java @@ -25,7 +25,6 @@ import com.netflix.genie.common.external.dtos.v4.ArchiveStatus; import com.netflix.genie.common.external.dtos.v4.JobStatus; import com.netflix.genie.common.internal.exceptions.checked.GenieJobResolutionException; -import com.netflix.genie.common.internal.exceptions.unchecked.GenieInvalidStatusException; import com.netflix.genie.common.internal.tracing.brave.BraveTracingComponents; import com.netflix.genie.web.agent.launchers.AgentLauncher; import com.netflix.genie.web.data.services.DataServices; @@ -149,7 +148,10 @@ public String launchJob( MetricsUtils.addFailureTagsWithException(tags, t); this.persistenceService.updateJobArchiveStatus(jobId, ArchiveStatus.NO_FILES); - if (this.updateJobStatus(jobId, JobStatus.FAILED, message, INITIAL_ATTEMPT) != JobStatus.FAILED) { + if ( + this.updateJobStatus(jobId, JobStatus.RESERVED, JobStatus.FAILED, message, INITIAL_ATTEMPT) + != JobStatus.FAILED + ) { log.error("Updating status to failed didn't succeed"); } throw t; // Caught below for metrics gathering @@ -161,6 +163,7 @@ public String launchJob( try { final JobStatus updatedStatus = this.updateJobStatus( jobId, + JobStatus.RESOLVED, JobStatus.ACCEPTED, ACCEPTED_MESSAGE, INITIAL_ATTEMPT @@ -186,7 +189,7 @@ public String launchJob( launcherExt = launcher.launchAgent(resolvedJob, requestedLauncherExt); } catch (final AgentLaunchException e) { this.persistenceService.updateJobArchiveStatus(jobId, ArchiveStatus.NO_FILES); - this.updateJobStatus(jobId, JobStatus.FAILED, e.getMessage(), INITIAL_ATTEMPT); + this.updateJobStatus(jobId, JobStatus.ACCEPTED, JobStatus.FAILED, e.getMessage(), INITIAL_ATTEMPT); // TODO: How will we get the ID back to the user? Should we add it to an exception? We don't get // We don't get the ID until after saveJobSubmission so if that fails we'd still return nothing // Probably need multiple exceptions to be thrown from this API (if we go with checked) @@ -276,12 +279,18 @@ private AgentLauncher selectLauncher( */ private JobStatus updateJobStatus( final String jobId, + final JobStatus expectedStatus, final JobStatus desiredStatus, final String desiredStatusMessage, final int attemptNumber ) throws NotFoundException { final int nextAttemptNumber = attemptNumber + 1; - final JobStatus currentStatus = this.persistenceService.getJobStatus(jobId); + final JobStatus currentStatus = this.persistenceService.updateJobStatus( + jobId, + expectedStatus, + desiredStatus, + desiredStatusMessage + ); if (currentStatus.isFinished()) { log.info( "Won't change job status of {} from {} to {} desired status as {} is already a final status", @@ -291,32 +300,36 @@ private JobStatus updateJobStatus( currentStatus ); return currentStatus; + } else if (currentStatus == desiredStatus) { + log.debug("Successfully updated status of {} from {} to {}", jobId, expectedStatus, desiredStatus); + return currentStatus; } else { - try { - this.persistenceService.updateJobStatus(jobId, currentStatus, desiredStatus, desiredStatusMessage); - return desiredStatus; - } catch (final GenieInvalidStatusException e) { - log.error( - "Job {} status changed from expected {}. Couldn't update to {}. Attempt {}", + log.error( + "Job {} status changed from expected {} to {}. Couldn't update to {}. Attempt {}", + jobId, + expectedStatus, + currentStatus, + desiredStatus, + nextAttemptNumber + ); + // Recursive call that should break out if update is successful or job is now in a final state + // or if attempts reach the max attempts + if (nextAttemptNumber < MAX_STATUS_UPDATE_ATTEMPTS) { + return this.updateJobStatus( jobId, currentStatus, desiredStatus, + desiredStatusMessage, nextAttemptNumber ); - // Recursive call that should break out if update is successful or job is now in a final state - // or if attempts reach the max attempts - if (nextAttemptNumber < MAX_STATUS_UPDATE_ATTEMPTS) { - return this.updateJobStatus(jobId, desiredStatus, desiredStatusMessage, attemptNumber + 1); - } else { - // breakout condition, stop attempting to update DB - log.error( - "Out of attempts to update job {} status to {}. Unable to complete status update", - jobId, - desiredStatus, - e - ); - return currentStatus; - } + } else { + // breakout condition, stop attempting to update DB + log.error( + "Out of attempts to update job {} status to {}. Unable to complete status update", + jobId, + desiredStatus + ); + return currentStatus; } } } diff --git a/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/JobLaunchServiceImplSpec.groovy b/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/JobLaunchServiceImplSpec.groovy index 77f4a89482a..75e7a037423 100644 --- a/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/JobLaunchServiceImplSpec.groovy +++ b/genie-web/src/test/groovy/com/netflix/genie/web/services/impl/JobLaunchServiceImplSpec.groovy @@ -107,8 +107,7 @@ class JobLaunchServiceImplSpec extends Specification { 1 * this.span.annotate(JobLaunchServiceImpl.SAVED_JOB_SUBMISSION_ANNOTATION) 1 * this.jobResolverService.resolveJob(jobId) >> resolvedJob 1 * this.span.annotate(JobLaunchServiceImpl.RESOLVED_JOB_ANNOTATION) - 1 * this.persistenceService.getJobStatus(jobId) >> JobStatus.RESOLVED - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) >> JobStatus.ACCEPTED 1 * this.span.annotate(JobLaunchServiceImpl.MARKED_JOB_ACCEPTED_ANNOTATION) 1 * this.agentLauncherSelector.getAgentLaunchers() >> [agentLauncher] 1 * jobSubmission.getJobRequest() >> jobRequest @@ -194,13 +193,12 @@ class JobLaunchServiceImplSpec extends Specification { throw new GenieJobResolutionException("fail") } 0 * this.span.annotate(JobLaunchServiceImpl.RESOLVED_JOB_ANNOTATION) - 1 * this.persistenceService.getJobStatus(jobId) >> JobStatus.RESERVED 1 * this.persistenceService.updateJobStatus( jobId, JobStatus.RESERVED, JobStatus.FAILED, JobStatusMessages.FAILED_TO_RESOLVE_JOB - ) + ) >> JobStatus.FAILED 1 * this.persistenceService.updateJobArchiveStatus(jobId, ArchiveStatus.NO_FILES) 0 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) 0 * agentLauncher.launchAgent(_ as ResolvedJob, requestedLauncherExt) @@ -219,13 +217,12 @@ class JobLaunchServiceImplSpec extends Specification { throw new GenieJobResolutionRuntimeException("fail") } 0 * this.span.annotate(JobLaunchServiceImpl.RESOLVED_JOB_ANNOTATION) - 1 * this.persistenceService.getJobStatus(jobId) >> JobStatus.RESERVED 1 * this.persistenceService.updateJobStatus( jobId, JobStatus.RESERVED, JobStatus.FAILED, JobStatusMessages.RESOLUTION_RUNTIME_ERROR - ) + ) >> JobStatus.FAILED 1 * this.persistenceService.updateJobArchiveStatus(jobId, ArchiveStatus.NO_FILES) 0 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) 0 * agentLauncher.launchAgent(_ as ResolvedJob, requestedLauncherExt) @@ -242,8 +239,7 @@ class JobLaunchServiceImplSpec extends Specification { 1 * this.span.annotate(JobLaunchServiceImpl.SAVED_JOB_SUBMISSION_ANNOTATION) 1 * this.jobResolverService.resolveJob(jobId) >> resolvedJob 1 * this.span.annotate(JobLaunchServiceImpl.RESOLVED_JOB_ANNOTATION) - 1 * this.persistenceService.getJobStatus(jobId) >> JobStatus.KILLED - 0 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) >> JobStatus.KILLED 0 * this.span.annotate(JobLaunchServiceImpl.MARKED_JOB_ACCEPTED_ANNOTATION) 1 * this.persistenceService.updateJobArchiveStatus(jobId, ArchiveStatus.NO_FILES) 0 * agentLauncher.launchAgent(_ as ResolvedJob, requestedLauncherExt) @@ -260,13 +256,11 @@ class JobLaunchServiceImplSpec extends Specification { 1 * this.span.annotate(JobLaunchServiceImpl.SAVED_JOB_SUBMISSION_ANNOTATION) 1 * this.jobResolverService.resolveJob(jobId) >> resolvedJob 1 * this.span.annotate(JobLaunchServiceImpl.RESOLVED_JOB_ANNOTATION) - JobLaunchServiceImpl.MAX_STATUS_UPDATE_ATTEMPTS * this.persistenceService.getJobStatus(jobId) >> JobStatus.RESOLVED - JobLaunchServiceImpl.MAX_STATUS_UPDATE_ATTEMPTS * this.persistenceService.updateJobStatus( - jobId, - JobStatus.RESOLVED, - JobStatus.ACCEPTED, - _ as String - ) >> { throw new GenieInvalidStatusException() } + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) >> JobStatus.RESERVED + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESERVED, JobStatus.ACCEPTED, _ as String) >> JobStatus.CLAIMED + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.CLAIMED, JobStatus.ACCEPTED, _ as String) >> JobStatus.INIT + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.INIT, JobStatus.ACCEPTED, _ as String) >> JobStatus.RUNNING + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RUNNING, JobStatus.ACCEPTED, _ as String) >> JobStatus.RESERVED 0 * this.span.annotate(JobLaunchServiceImpl.MARKED_JOB_ACCEPTED_ANNOTATION) 1 * this.persistenceService.updateJobArchiveStatus(jobId, ArchiveStatus.NO_FILES) 0 * agentLauncher.launchAgent(_ as ResolvedJob, requestedLauncherExt) @@ -284,15 +278,14 @@ class JobLaunchServiceImplSpec extends Specification { 1 * this.span.annotate(JobLaunchServiceImpl.SAVED_JOB_SUBMISSION_ANNOTATION) 1 * this.jobResolverService.resolveJob(jobId) >> resolvedJob 1 * this.span.annotate(JobLaunchServiceImpl.RESOLVED_JOB_ANNOTATION) - 2 * this.persistenceService.getJobStatus(jobId) >>> [JobStatus.RESOLVED, JobStatus.ACCEPTED] - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) >> JobStatus.ACCEPTED 1 * this.span.annotate(JobLaunchServiceImpl.MARKED_JOB_ACCEPTED_ANNOTATION) 1 * this.persistenceService.getRequestedLauncherExt(jobId) >> requestedLauncherExt 1 * this.agentLauncherSelector.getAgentLaunchers() >> [agentLauncher] 1 * this.agentLauncherSelector.select(_ as AgentLauncherSelectionContext) >> { throw new ResourceSelectionException("...") } - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.ACCEPTED, JobStatus.FAILED, _ as String) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.ACCEPTED, JobStatus.FAILED, _ as String) >> JobStatus.FAILED 0 * agentLauncher.launchAgent(resolvedJob, requestedLauncherExt) 1 * this.span.annotate(JobLaunchServiceImpl.END_LAUNCH_JOB_ANNOTATION) thrown(AgentLaunchException) @@ -307,8 +300,7 @@ class JobLaunchServiceImplSpec extends Specification { 1 * this.span.annotate(JobLaunchServiceImpl.SAVED_JOB_SUBMISSION_ANNOTATION) 1 * this.jobResolverService.resolveJob(jobId) >> resolvedJob 1 * this.span.annotate(JobLaunchServiceImpl.RESOLVED_JOB_ANNOTATION) - 2 * this.persistenceService.getJobStatus(jobId) >>> [JobStatus.RESOLVED, JobStatus.ACCEPTED] - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) >> JobStatus.ACCEPTED 1 * this.span.annotate(JobLaunchServiceImpl.MARKED_JOB_ACCEPTED_ANNOTATION) 1 * this.persistenceService.getRequestedLauncherExt(jobId) >> requestedLauncherExt 1 * this.agentLauncherSelector.getAgentLaunchers() >> [agentLauncher] @@ -316,7 +308,7 @@ class JobLaunchServiceImplSpec extends Specification { 1 * selectionResult.getSelectedResource() >> Optional.empty() 1 * selectionResult.getSelectionRationale() >> Optional.empty() 0 * this.span.annotate(JobLaunchServiceImpl.LAUNCHED_AGENT_ANNOTATION) - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.ACCEPTED, JobStatus.FAILED, _ as String) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.ACCEPTED, JobStatus.FAILED, _ as String) >> JobStatus.FAILED 1 * this.persistenceService.updateJobArchiveStatus(jobId, ArchiveStatus.NO_FILES) 0 * agentLauncher.launchAgent(resolvedJob, requestedLauncherExt) 1 * this.span.annotate(JobLaunchServiceImpl.END_LAUNCH_JOB_ANNOTATION) @@ -332,8 +324,7 @@ class JobLaunchServiceImplSpec extends Specification { 1 * this.span.annotate(JobLaunchServiceImpl.SAVED_JOB_SUBMISSION_ANNOTATION) 1 * this.jobResolverService.resolveJob(jobId) >> resolvedJob 1 * this.span.annotate(JobLaunchServiceImpl.RESOLVED_JOB_ANNOTATION) - 2 * this.persistenceService.getJobStatus(jobId) >>> [JobStatus.RESOLVED, JobStatus.ACCEPTED] - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) >> JobStatus.ACCEPTED 1 * this.span.annotate(JobLaunchServiceImpl.MARKED_JOB_ACCEPTED_ANNOTATION) 1 * this.persistenceService.getRequestedLauncherExt(jobId) >> requestedLauncherExt 1 * this.agentLauncherSelector.getAgentLaunchers() >> [agentLauncher] @@ -343,7 +334,7 @@ class JobLaunchServiceImplSpec extends Specification { throw new AgentLaunchException("that didn't work") } 0 * this.span.annotate(JobLaunchServiceImpl.LAUNCHED_AGENT_ANNOTATION) - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.ACCEPTED, JobStatus.FAILED, _ as String) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.ACCEPTED, JobStatus.FAILED, _ as String) >> JobStatus.FAILED 1 * this.persistenceService.updateJobArchiveStatus(jobId, ArchiveStatus.NO_FILES) 1 * this.span.annotate(JobLaunchServiceImpl.END_LAUNCH_JOB_ANNOTATION) thrown(AgentLaunchException) @@ -358,8 +349,7 @@ class JobLaunchServiceImplSpec extends Specification { 1 * this.span.annotate(JobLaunchServiceImpl.SAVED_JOB_SUBMISSION_ANNOTATION) 1 * this.jobResolverService.resolveJob(jobId) >> resolvedJob 1 * this.span.annotate(JobLaunchServiceImpl.RESOLVED_JOB_ANNOTATION) - 1 * this.persistenceService.getJobStatus(jobId) >> JobStatus.RESOLVED - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.ACCEPTED, _ as String) >> JobStatus.ACCEPTED 1 * this.span.annotate(JobLaunchServiceImpl.MARKED_JOB_ACCEPTED_ANNOTATION) 1 * this.persistenceService.getRequestedLauncherExt(jobId) >> requestedLauncherExt 1 * this.agentLauncherSelector.getAgentLaunchers() >> [agentLauncher] @@ -376,57 +366,58 @@ class JobLaunchServiceImplSpec extends Specification { def "update job status works as expected"() { def jobId = UUID.randomUUID().toString() + def expectedStatus = JobStatus.RESOLVED def desiredStatus = JobStatus.ACCEPTED def desiredMessage = UUID.randomUUID().toString() def attemptNumber = 0 when: "The current status is already finished" - def jobStatus = this.service.updateJobStatus(jobId, desiredStatus, desiredMessage, attemptNumber) + def jobStatus = this.service.updateJobStatus( + jobId, + expectedStatus, + desiredStatus, + desiredMessage, + attemptNumber + ) then: "Nothing happens" - 1 * this.persistenceService.getJobStatus(jobId) >> JobStatus.KILLED - 0 * this.persistenceService.updateJobStatus(_ as String, _ as JobStatus, _ as JobStatus, _ as String) + 1 * this.persistenceService.updateJobStatus( + jobId, + JobStatus.RESOLVED, + JobStatus.ACCEPTED, + desiredMessage + ) >> JobStatus.KILLED jobStatus == JobStatus.KILLED noExceptionThrown() when: "The current status isn't finished but changes when updated is attempted" - jobStatus = this.service.updateJobStatus(jobId, desiredStatus, desiredMessage, attemptNumber) + jobStatus = this.service.updateJobStatus(jobId, expectedStatus, desiredStatus, desiredMessage, attemptNumber) then: "Method is retried but finished status is respected" - 2 * this.persistenceService.getJobStatus(jobId) >>> [JobStatus.RESERVED, JobStatus.KILLED] - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESERVED, desiredStatus, desiredMessage) >> { - throw new GenieInvalidStatusException("killed") - } + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, desiredStatus, desiredMessage) >> JobStatus.RESERVED + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESERVED, desiredStatus, desiredMessage) >> JobStatus.KILLED jobStatus == JobStatus.KILLED noExceptionThrown() - when: "The current status isn't finished but changes when updated is attempted" - jobStatus = this.service.updateJobStatus(jobId, desiredStatus, desiredMessage, attemptNumber) + when: "The current status isn't finished but changes when update is attempted" + jobStatus = this.service.updateJobStatus(jobId, JobStatus.RESOLVED, desiredStatus, desiredMessage, attemptNumber) then: "Method is retried and succeeds" - 2 * this.persistenceService.getJobStatus(jobId) >>> [JobStatus.RESERVED, JobStatus.RESOLVED] - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESERVED, desiredStatus, desiredMessage) >> { - throw new GenieInvalidStatusException("resolved") - } - 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, desiredStatus, desiredMessage) + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, desiredStatus, desiredMessage) >> JobStatus.RESERVED + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESERVED, desiredStatus, desiredMessage) >> JobStatus.ACCEPTED jobStatus == desiredStatus noExceptionThrown() when: "Max retries are exceeded" - jobStatus = this.service.updateJobStatus(jobId, desiredStatus, desiredMessage, attemptNumber) + jobStatus = this.service.updateJobStatus(jobId, JobStatus.RESERVED, JobStatus.INVALID, desiredMessage, attemptNumber) then: "Exception is swallowed and failure returned" - JobLaunchServiceImpl.MAX_STATUS_UPDATE_ATTEMPTS * this.persistenceService.getJobStatus(jobId) >> - JobStatus.RESERVED - JobLaunchServiceImpl.MAX_STATUS_UPDATE_ATTEMPTS * this.persistenceService.updateJobStatus( - jobId, - JobStatus.RESERVED, - desiredStatus, - desiredMessage - ) >> { - throw new GenieInvalidStatusException("resolved") - } + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESERVED, JobStatus.INVALID, desiredMessage) >> JobStatus.RESOLVED + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.RESOLVED, JobStatus.INVALID, desiredMessage) >> JobStatus.ACCEPTED + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.ACCEPTED, JobStatus.INVALID, desiredMessage) >> JobStatus.CLAIMED + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.CLAIMED, JobStatus.INVALID, desiredMessage) >> JobStatus.INIT + 1 * this.persistenceService.updateJobStatus(jobId, JobStatus.INIT, JobStatus.INVALID, desiredMessage) >> JobStatus.RUNNING noExceptionThrown() - jobStatus == JobStatus.RESERVED + jobStatus == JobStatus.RUNNING } } diff --git a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsTest.java b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsTest.java index 56bef2791b7..513e5c7f122 100644 --- a/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsTest.java +++ b/genie-web/src/test/java/com/netflix/genie/web/data/services/impl/jpa/JpaPersistenceServiceImplJobsTest.java @@ -307,16 +307,11 @@ void testClaimJobValidBehavior() throws GenieCheckedException { } @Test - void testUpdateJobStatusErrorCases() { + void testUpdateJobStatusErrorCases() throws NotFoundException { final String id = UUID.randomUUID().toString(); Assertions - .assertThatExceptionOfType(GenieInvalidStatusException.class) - .isThrownBy(() -> this.persistenceService.updateJobStatus( - id, - JobStatus.CLAIMED, - JobStatus.CLAIMED, - null) - ); + .assertThat(this.persistenceService.updateJobStatus(id, JobStatus.CLAIMED, JobStatus.CLAIMED, null)) + .isEqualTo(JobStatus.CLAIMED); Mockito .when(this.jobRepository.findByUniqueId(id)) @@ -325,10 +320,10 @@ void testUpdateJobStatusErrorCases() { Assertions .assertThatExceptionOfType(NotFoundException.class) .isThrownBy(() -> this.persistenceService.updateJobStatus( - id, - JobStatus.CLAIMED, - JobStatus.INIT, - null + id, + JobStatus.CLAIMED, + JobStatus.INIT, + null ) ); @@ -338,17 +333,9 @@ void testUpdateJobStatusErrorCases() { .thenReturn(Optional.of(jobEntity)); Mockito.when(jobEntity.getStatus()).thenReturn(JobStatus.INIT.name()); - Assertions - .assertThatExceptionOfType(GenieInvalidStatusException.class) - .isThrownBy( - () -> this.persistenceService.updateJobStatus( - id, - JobStatus.CLAIMED, - JobStatus.INIT, - null - ) - ); + .assertThat(this.persistenceService.updateJobStatus(id, JobStatus.CLAIMED, JobStatus.INIT, null)) + .isEqualTo(JobStatus.INIT); } @Test