From 28be0b85d64b99d410ef772ca869d5c3d76783a9 Mon Sep 17 00:00:00 2001 From: tyner Date: Wed, 3 Apr 2024 10:44:08 -0400 Subject: [PATCH 01/13] - Implemented GATE_WAITING state for the batch2 state machine. - This will be the initial status for all workchunks of a gated job. - made compatible with the equivalent "fake QUEUED" state in the Old batch2 implementation. - Updated corresponding docs. - added corresponding tests and changelog --- ...ch2-framework-with-gate_waiting-state.yaml | 7 + .../docs/server_jpa_batch/batch2_states.md | 9 +- .../docs/server_jpa_batch/introduction.md | 6 +- .../jpa/batch2/JpaJobPersistenceImpl.java | 62 ++++++- .../dao/data/IBatch2WorkChunkRepository.java | 9 + .../jpa/batch2/JpaJobPersistenceImplTest.java | 31 ++++ .../jpa/batch2/JpaJobPersistenceImplTest.java | 169 ++++++++++++++---- ...tractIJobPersistenceSpecificationTest.java | 14 +- .../batch2/test/IJobMaintenanceActions.java | 81 +++++---- .../fhir/batch2/test/IWorkChunkCommon.java | 8 +- .../test/IWorkChunkStateTransitions.java | 27 ++- .../batch2/test/IWorkChunkStorageTests.java | 21 +-- .../JobMaintenanceStateInformation.java | 3 +- .../uhn/fhir/batch2/api/IJobPersistence.java | 24 +++ .../batch2/api/IWorkChunkPersistence.java | 3 +- .../fhir/batch2/coordinator/JobDataSink.java | 8 +- .../maintenance/JobInstanceProcessor.java | 68 ++++--- .../batch2/model/WorkChunkCreateEvent.java | 11 +- .../batch2/model/WorkChunkStatusEnum.java | 3 + .../batch2/progress/InstanceProgress.java | 1 + 20 files changed, 438 insertions(+), 127 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5818-update-batch2-framework-with-gate_waiting-state.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5818-update-batch2-framework-with-gate_waiting-state.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5818-update-batch2-framework-with-gate_waiting-state.yaml new file mode 100644 index 00000000000..852d110800a --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5818-update-batch2-framework-with-gate_waiting-state.yaml @@ -0,0 +1,7 @@ +--- +type: add +issue: 5818 +title: "Added another state to the Batch2 work chunk state machine: GATE_WAITING. + This work chunk state will be the initial state on creation for gated jobs. + Once all chunks are completed for the previous step, they will transition to READY. +" diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md index 9d1afea884b..1db1e9f1d18 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md @@ -56,10 +56,11 @@ stateDiagram-v2 state FAILED state COMPLETED direction LR - [*] --> READY : on create - normal - [*] --> GATED : on create - gated - GATED --> READY : on create - gated - READY --> QUEUED : placed on kafka (maint.) + [*] --> READY : on create - normal + [*] --> GATE_WAITING : on create - gated + GATE_WAITING --> READY : on gate release - gated (new) + QUEUED --> READY : on gate release - gated (for compatibility with old "fake QUEUED" state) + READY --> QUEUED : placed on kafka (maint.) %% worker processing states QUEUED --> on_receive : on deque by worker diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md index f50550ea6a1..e05cb1c54ec 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md @@ -19,14 +19,14 @@ A HAPI-FHIR batch job definition consists of a job name, version, parameter json After a job has been defined, *instances* of that job can be submitted for batch processing by populating a `JobInstanceStartRequest` with the job name and job parameters json and then submitting that request to the Batch Job Coordinator. The Batch Job Coordinator will then store two records in the database: -- Job Instance with status QUEUED: that is the parent record for all data concerning this job -- Batch Work Chunk with status READY: this describes the first "chunk" of work required for this job. The first Batch Work Chunk contains no data. +- Job Instance with status `QUEUED`: that is the parent record for all data concerning this job +- Batch Work Chunk with status `READY`/`GATE_WAITING`: this describes the first "chunk" of work required for this job. The first Batch Work Chunk contains no data. The initial status of the work chunk will be `READY` or `GATE_WAITING` for non-gated and gated batch jobs, respectively. Please refer to [Gated Execution](#gated-execution) for more explanation on gated batch jobs. ### The Maintenance Job A Scheduled Job runs periodically (once a minute). For each Job Instance in the database, it: -1. Moves all `READY` work chunks into the `QUEUED` state and publishes a message to the Batch Notification Message Channel to inform worker threads that a work chunk is now ready for processing. \* +1. Moves all `READY` work chunks into the `QUEUED` state and publishes a message to the Batch Notification Message Channel to inform worker threads that a work chunk is now ready for processing. For gated batch jobs, the maintenance also moves all `GATE_WAITING` work chunks into `READY` when the current batch step is ready to advance. \* 1. Calculates job progress (% of work chunks in `COMPLETE` status). If the job is finished, purges any left over work chunks still in the database. 1. Cleans up any complete, failed, or cancelled jobs that need to be removed. 1. Moves any gated jobs onto their next step when complete. diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java index 3851926d49b..89f56e671a4 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java @@ -128,7 +128,10 @@ public class JpaJobPersistenceImpl implements IJobPersistence { entity.setSerializedData(theBatchWorkChunk.serializedData); entity.setCreateTime(new Date()); entity.setStartTime(new Date()); - entity.setStatus(WorkChunkStatusEnum.READY); + // set to GATE_WAITING if job is gated, to READY if not + entity.setStatus( + theBatchWorkChunk.isGatedExecution ? WorkChunkStatusEnum.GATE_WAITING : WorkChunkStatusEnum.READY); + ourLog.debug("Create work chunk {}/{}/{}", entity.getInstanceId(), entity.getId(), entity.getTargetStepId()); ourLog.trace( "Create work chunk data {}/{}: {}", entity.getInstanceId(), entity.getId(), entity.getSerializedData()); @@ -575,4 +578,61 @@ public class JpaJobPersistenceImpl implements IJobPersistence { myInterceptorBroadcaster.callHooks(Pointcut.STORAGE_PRESTORAGE_BATCH_JOB_CREATE, params); } } + + @Override + @Transactional(propagation = Propagation.REQUIRES_NEW) + public boolean advanceJobStepAndUpdateChunkStatus(String theJobInstanceId, String theNextStepId) { + boolean changed = updateInstance(theJobInstanceId, instance -> { + if (instance.getCurrentGatedStepId().equals(theNextStepId)) { + // someone else beat us here. No changes + return false; + } + ourLog.debug("Moving gated instance {} to the next step {}.", theJobInstanceId, theNextStepId); + instance.setCurrentGatedStepId(theNextStepId); + return true; + }); + + if (changed) { + ourLog.debug( + "Updating chunk status from GATE_WAITING to READY for gated instance {} in step {}.", + theJobInstanceId, + theNextStepId); + // when we reach here, the current step id is equal to theNextStepId + myWorkChunkRepository.updateAllChunksForStepWithStatus( + theJobInstanceId, theNextStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.GATE_WAITING); + + // In the old code, gated jobs' workchunks are created in status QUEUED but not actually queued for the + // workers. + // In order to keep them compatible, turn QUEUED chunks into READY, too. + // TODO: remove this when we are certain that no one is still running the old code. + int numChanged = myWorkChunkRepository.updateAllChunksForStepWithStatus( + theJobInstanceId, theNextStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.QUEUED); + if (numChanged > 0) { + ourLog.debug( + "Updated {} chunks of gated instance {} for step {} from fake QUEUED to READY.", + numChanged, + theJobInstanceId, + theNextStepId); + } + } + + return changed; + } + + @Override + @Transactional(propagation = Propagation.REQUIRES_NEW) + public int updateAllChunksForStepWithStatus( + String theJobInstanceId, + String theStepId, + WorkChunkStatusEnum theNewStatus, + WorkChunkStatusEnum theOldStatus) { + ourLog.debug( + "Updating chunk status from {} to {} for gated instance {} in step {}.", + theOldStatus, + theNewStatus, + theJobInstanceId, + theStepId); + return myWorkChunkRepository.updateAllChunksForStepWithStatus( + theJobInstanceId, theStepId, theNewStatus, theOldStatus); + } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java index 2273fb3aa05..93cd023c839 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java @@ -109,6 +109,15 @@ public interface IBatch2WorkChunkRepository @Param("newStatus") WorkChunkStatusEnum theNewStatus, @Param("oldStatus") WorkChunkStatusEnum theOldStatus); + @Modifying + @Query( + "UPDATE Batch2WorkChunkEntity e SET e.myStatus = :newStatus WHERE e.myInstanceId = :instanceId AND e.myTargetStepId = :stepId AND e.myStatus = :oldStatus") + int updateAllChunksForStepWithStatus( + @Param("instanceId") String theInstanceId, + @Param("stepId") String theStepId, + @Param("newStatus") WorkChunkStatusEnum theNewStatus, + @Param("oldStatus") WorkChunkStatusEnum theOldStatus); + @Modifying @Query("DELETE FROM Batch2WorkChunkEntity e WHERE e.myInstanceId = :instanceId") int deleteAllForInstance(@Param("instanceId") String theInstanceId); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index ba013f714bc..1b1cb585d51 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -4,6 +4,7 @@ import ca.uhn.fhir.batch2.api.JobOperationResultJson; import ca.uhn.fhir.batch2.model.FetchJobInstancesRequest; import ca.uhn.fhir.batch2.model.JobInstance; import ca.uhn.fhir.batch2.model.StatusEnum; +import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; import ca.uhn.fhir.jpa.dao.data.IBatch2JobInstanceRepository; import ca.uhn.fhir.jpa.dao.data.IBatch2WorkChunkRepository; import ca.uhn.fhir.jpa.dao.tx.IHapiTransactionService; @@ -173,6 +174,36 @@ class JpaJobPersistenceImplTest { assertEquals(instance.getInstanceId(), retInstance.get().getInstanceId()); } + @Test + void advanceJobStepAndUpdateChunkStatus_validRequest_callsPersistenceUpdateAndReturnsChanged() { + // setup + String instanceId = "jobId"; + String nextStepId = "nextStep"; + + // execute + mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, nextStepId); + + // verify + verify(mySvc).updateInstance(instanceId, any()); + verify(myWorkChunkRepository).updateAllChunksForStepWithStatus(instanceId, nextStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.GATE_WAITING); + verify(myWorkChunkRepository).updateAllChunksForStepWithStatus(instanceId, nextStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.QUEUED); + } + + @Test + void updateAllChunksForStepWithStatus_validRequest_callsPersistenceUpdateAndReturnsChanged() { + // setup + String instanceId = "jobId"; + String nextStepId = "nextStep"; + WorkChunkStatusEnum expectedNewStatus = WorkChunkStatusEnum.READY; + WorkChunkStatusEnum expectedOldStatus = WorkChunkStatusEnum.GATE_WAITING; + + // execute + mySvc.updateAllChunksForStepWithStatus(instanceId, nextStepId, expectedNewStatus, expectedOldStatus); + + // verify + verify(myWorkChunkRepository).updateAllChunksForStepWithStatus(instanceId, nextStepId, expectedNewStatus, expectedOldStatus); + } + private JobInstance createJobInstanceFromEntity(Batch2JobInstanceEntity theEntity) { return JobInstanceUtil.fromEntityToInstance(theEntity); } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index a5e83893195..f2da87cd00f 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -38,6 +38,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestMethodOrder; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.MethodSource; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Import; @@ -79,6 +80,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { public static final String JOB_DEFINITION_ID = "definition-id"; public static final String TARGET_STEP_ID = TestJobDefinitionUtils.FIRST_STEP_ID; + public static final String LAST_STEP_ID = TestJobDefinitionUtils.LAST_STEP_ID; public static final String DEF_CHUNK_ID = "definition-chunkId"; public static final String STEP_CHUNK_ID = TestJobDefinitionUtils.FIRST_STEP_ID; public static final int JOB_DEF_VER = 1; @@ -110,7 +112,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { JobInstance instance = createInstance(); String instanceId = mySvc.storeNewInstance(instance); for (int i = 0; i < 10; i++) { - storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, i, JsonUtil.serialize(new NdJsonFileJson().setNdJsonText("{}"))); + storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, i, JsonUtil.serialize(new NdJsonFileJson().setNdJsonText("{}")), false); } // Execute @@ -125,8 +127,8 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { }); } - private String storeWorkChunk(String theJobDefinitionId, String theTargetStepId, String theInstanceId, int theSequence, String theSerializedData) { - WorkChunkCreateEvent batchWorkChunk = new WorkChunkCreateEvent(theJobDefinitionId, TestJobDefinitionUtils.TEST_JOB_VERSION, theTargetStepId, theInstanceId, theSequence, theSerializedData); + private String storeWorkChunk(String theJobDefinitionId, String theTargetStepId, String theInstanceId, int theSequence, String theSerializedData, boolean theGatedExecution) { + WorkChunkCreateEvent batchWorkChunk = new WorkChunkCreateEvent(theJobDefinitionId, TestJobDefinitionUtils.TEST_JOB_VERSION, theTargetStepId, theInstanceId, theSequence, theSerializedData, theGatedExecution); return mySvc.onWorkChunkCreate(batchWorkChunk); } @@ -240,8 +242,8 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { // Setup JobInstance instance = createInstance(); String instanceId = mySvc.storeNewInstance(instance); - storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA); - WorkChunkCreateEvent batchWorkChunk = new WorkChunkCreateEvent(JOB_DEFINITION_ID, JOB_DEF_VER, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA); + storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA, false); + WorkChunkCreateEvent batchWorkChunk = new WorkChunkCreateEvent(JOB_DEFINITION_ID, JOB_DEF_VER, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA, false); String chunkId = mySvc.onWorkChunkCreate(batchWorkChunk); Optional byId = myWorkChunkRepository.findById(chunkId); Batch2WorkChunkEntity entity = byId.get(); @@ -367,7 +369,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { @Test public void testUpdateTime() { // Setup - JobInstance instance = createInstance(); + JobInstance instance = createInstance(true, false); String instanceId = mySvc.storeNewInstance(instance); Date updateTime = runInTransaction(() -> new Date(myJobInstanceRepository.findById(instanceId).orElseThrow().getUpdateTime().getTime())); @@ -382,35 +384,128 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { assertNotEquals(updateTime, updateTime2); } + @Test + public void advanceJobStepAndUpdateChunkStatus_forGatedJob_updatesCurrentStepAndChunkStatus() { + // setup + JobInstance instance = createInstance(true, true); + String instanceId = mySvc.storeNewInstance(instance); + String chunkIdFirstStep = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null, true); + String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); + String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); + + runInTransaction(() -> assertEquals(TARGET_STEP_ID, myJobInstanceRepository.findById(instanceId).orElseThrow(IllegalArgumentException::new).getCurrentGatedStepId())); + + // execute + runInTransaction(() -> mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, LAST_STEP_ID)); + + // verify + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(chunkIdFirstStep).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkIdSecondStep1).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkIdSecondStep2).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(LAST_STEP_ID, myJobInstanceRepository.findById(instanceId).orElseThrow(IllegalArgumentException::new).getCurrentGatedStepId())); + } + + @Test + public void updateAllChunksForStepWithStatus_forGatedJob_updatesChunkStatus() { + // setup + WorkChunkStatusEnum expectedNoChangeStatus = WorkChunkStatusEnum.GATE_WAITING; + WorkChunkStatusEnum expectedChangedStatus = WorkChunkStatusEnum.COMPLETED; + JobInstance instance = createInstance(true, true); + String instanceId = mySvc.storeNewInstance(instance); + String chunkIdFirstStep = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null, true); + String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); + String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); + + runInTransaction(() -> assertEquals(expectedNoChangeStatus, myWorkChunkRepository.findById(chunkIdFirstStep).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(expectedNoChangeStatus, myWorkChunkRepository.findById(chunkIdSecondStep1).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(expectedNoChangeStatus, myWorkChunkRepository.findById(chunkIdSecondStep2).orElseThrow(IllegalArgumentException::new).getStatus())); + + // execute + runInTransaction(() -> mySvc.updateAllChunksForStepWithStatus(instanceId, LAST_STEP_ID, expectedChangedStatus, WorkChunkStatusEnum.GATE_WAITING)); + + // verify + runInTransaction(() -> assertEquals(expectedNoChangeStatus, myWorkChunkRepository.findById(chunkIdFirstStep).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(expectedChangedStatus, myWorkChunkRepository.findById(chunkIdSecondStep1).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(expectedChangedStatus, myWorkChunkRepository.findById(chunkIdSecondStep2).orElseThrow(IllegalArgumentException::new).getStatus())); + } + @Test public void testFetchUnknownWork() { assertFalse(myWorkChunkRepository.findById("FOO").isPresent()); } - @Test - public void testStoreAndFetchWorkChunk_NoData() { - JobInstance instance = createInstance(true, false); + @ParameterizedTest + @CsvSource({ + "false, READY, QUEUED", + "true, GATE_WAITING, QUEUED" + }) + public void testStoreAndFetchWorkChunk_withOrWithoutGatedExecution_createdAndTransitionToExpectedStatus(boolean theGatedExecution, WorkChunkStatusEnum theExpectedStatusOnCreate, WorkChunkStatusEnum theExpectedStatusAfterTransition) { + // setup + JobInstance instance = createInstance(true, theGatedExecution); String instanceId = mySvc.storeNewInstance(instance); - String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null); - - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + // execute & verify + String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null, theGatedExecution); + runInTransaction(() -> assertEquals(theExpectedStatusOnCreate, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(theExpectedStatusAfterTransition, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + + if (WorkChunkStatusEnum.READY.equals(theExpectedStatusAfterTransition)) { + myBatch2JobHelper.runMaintenancePass(); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + } WorkChunk chunk = mySvc.onWorkChunkDequeue(id).orElseThrow(IllegalArgumentException::new); assertNull(chunk.getData()); } + @Test + public void testStoreAndFetchWorkChunk_withGatedJobMultipleChunk_correctTransitions() { + // setup + boolean isGatedExecution = true; + String expectedFirstChunkData = "IAmChunk1"; + String expectedSecondChunkData = "IAmChunk2"; + JobInstance instance = createInstance(true, isGatedExecution); + String instanceId = mySvc.storeNewInstance(instance); + + // execute & verify + String firstChunkId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, expectedFirstChunkData, isGatedExecution); + String secondChunkId = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, expectedSecondChunkData, isGatedExecution); + + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + + myBatch2JobHelper.runMaintenancePass(); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + + WorkChunk actualFirstChunkData = mySvc.onWorkChunkDequeue(firstChunkId).orElseThrow(IllegalArgumentException::new); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + assertEquals(expectedFirstChunkData, actualFirstChunkData.getData()); + + mySvc.onWorkChunkCompletion(new WorkChunkCompletionEvent(firstChunkId, 50, 0)); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.COMPLETED, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + + myBatch2JobHelper.runMaintenancePass(); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.COMPLETED, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + + WorkChunk actualSecondChunkData = mySvc.onWorkChunkDequeue(secondChunkId).orElseThrow(IllegalArgumentException::new); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + assertEquals(expectedSecondChunkData, actualSecondChunkData.getData()); + } + @Test void testStoreAndFetchChunksForInstance_NoData() { // given + boolean isGatedExecution = false; JobInstance instance = createInstance(); String instanceId = mySvc.storeNewInstance(instance); - String queuedId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, "some data"); - String erroredId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 1, "some more data"); - String completedId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 2, "some more data"); + String queuedId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, "some data", isGatedExecution); + String erroredId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 1, "some more data", isGatedExecution); + String completedId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 2, "some more data", isGatedExecution); mySvc.onWorkChunkDequeue(erroredId); WorkChunkErrorEvent parameters = new WorkChunkErrorEvent(erroredId, "Our error message"); @@ -468,17 +563,26 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { } - - @Test - public void testStoreAndFetchWorkChunk_WithData() { - JobInstance instance = createInstance(true, false); + @ParameterizedTest + @CsvSource({ + "false, READY, QUEUED", + "true, GATE_WAITING, QUEUED" + }) + public void testStoreAndFetchWorkChunk_WithData(boolean theGatedExecution, WorkChunkStatusEnum theExpectedCreatedStatus, WorkChunkStatusEnum theExpectedTransitionStatus) { + JobInstance instance = createInstance(true, theGatedExecution); String instanceId = mySvc.storeNewInstance(instance); - String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA); + String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA, theGatedExecution); assertNotNull(id); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(theExpectedCreatedStatus, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(theExpectedTransitionStatus, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + + if (WorkChunkStatusEnum.READY.equals(theExpectedTransitionStatus)){ + myBatch2JobHelper.runMaintenancePass(); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + } + WorkChunk chunk = mySvc.onWorkChunkDequeue(id).orElseThrow(IllegalArgumentException::new); assertEquals(36, chunk.getInstanceId().length()); assertEquals(JOB_DEFINITION_ID, chunk.getJobDefinitionId()); @@ -491,9 +595,10 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { @Test public void testMarkChunkAsCompleted_Success() { - JobInstance instance = createInstance(true, false); + boolean isGatedExecution = false; + JobInstance instance = createInstance(true, isGatedExecution); String instanceId = mySvc.storeNewInstance(instance); - String chunkId = storeWorkChunk(DEF_CHUNK_ID, STEP_CHUNK_ID, instanceId, SEQUENCE_NUMBER, CHUNK_DATA); + String chunkId = storeWorkChunk(DEF_CHUNK_ID, STEP_CHUNK_ID, instanceId, SEQUENCE_NUMBER, CHUNK_DATA, isGatedExecution); assertNotNull(chunkId); runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); @@ -528,9 +633,10 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { @Test public void testMarkChunkAsCompleted_Error() { - JobInstance instance = createInstance(true, false); + boolean isGatedExecution = false; + JobInstance instance = createInstance(true, isGatedExecution); String instanceId = mySvc.storeNewInstance(instance); - String chunkId = storeWorkChunk(JOB_DEFINITION_ID, TestJobDefinitionUtils.FIRST_STEP_ID, instanceId, SEQUENCE_NUMBER, null); + String chunkId = storeWorkChunk(JOB_DEFINITION_ID, TestJobDefinitionUtils.FIRST_STEP_ID, instanceId, SEQUENCE_NUMBER, null, isGatedExecution); assertNotNull(chunkId); runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); @@ -580,9 +686,10 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { @Test public void testMarkChunkAsCompleted_Fail() { - JobInstance instance = createInstance(true, false); + boolean isGatedExecution = false; + JobInstance instance = createInstance(true, isGatedExecution); String instanceId = mySvc.storeNewInstance(instance); - String chunkId = storeWorkChunk(DEF_CHUNK_ID, STEP_CHUNK_ID, instanceId, SEQUENCE_NUMBER, null); + String chunkId = storeWorkChunk(DEF_CHUNK_ID, STEP_CHUNK_ID, instanceId, SEQUENCE_NUMBER, null, isGatedExecution); assertNotNull(chunkId); runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); @@ -620,7 +727,8 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { "stepId", instanceId, 0, - "{}" + "{}", + false ); String id = mySvc.onWorkChunkCreate(chunk); chunkIds.add(id); @@ -698,6 +806,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { } ); + instance.setCurrentGatedStepId(jobDef.getFirstStepId()); } else { jobDef = TestJobDefinitionUtils.buildJobDefinition( JOB_DEFINITION_ID, diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java index 0f5c3f5017d..65f8388b4f4 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java @@ -91,6 +91,7 @@ public abstract class AbstractIJobPersistenceSpecificationTest implements IJobMa return mySvc; } + @Nonnull public JobDefinition withJobDefinition(boolean theIsGatedBoolean) { JobDefinition.Builder builder = JobDefinition.newBuilder() .setJobDefinitionId(theIsGatedBoolean ? GATED_JOB_DEFINITION_ID : JOB_DEFINITION_ID) @@ -165,11 +166,14 @@ public abstract class AbstractIJobPersistenceSpecificationTest implements IJobMa instance.setJobDefinitionVersion(JOB_DEF_VER); instance.setParameters(CHUNK_DATA); instance.setReport("TEST"); + if (jobDefinition.isGatedExecution()) { + instance.setCurrentGatedStepId(jobDefinition.getFirstStepId()); + } return instance; } - public String storeWorkChunk(String theJobDefinitionId, String theTargetStepId, String theInstanceId, int theSequence, String theSerializedData) { - WorkChunkCreateEvent batchWorkChunk = new WorkChunkCreateEvent(theJobDefinitionId, JOB_DEF_VER, theTargetStepId, theInstanceId, theSequence, theSerializedData); + public String storeWorkChunk(String theJobDefinitionId, String theTargetStepId, String theInstanceId, int theSequence, String theSerializedData, boolean theGatedExecution) { + WorkChunkCreateEvent batchWorkChunk = new WorkChunkCreateEvent(theJobDefinitionId, JOB_DEF_VER, theTargetStepId, theInstanceId, theSequence, theSerializedData, theGatedExecution); return mySvc.onWorkChunkCreate(batchWorkChunk); } @@ -226,7 +230,11 @@ public abstract class AbstractIJobPersistenceSpecificationTest implements IJobMa } public String createChunk(String theInstanceId) { - return storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, theInstanceId, 0, CHUNK_DATA); + return storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, theInstanceId, 0, CHUNK_DATA, false); + } + + public String createChunk(String theInstanceId, boolean theGatedExecution) { + return storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, theInstanceId, 0, CHUNK_DATA, theGatedExecution); } public void enableMaintenanceRunner(boolean theToEnable) { diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java index 6392c24428f..3824e35b81e 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java @@ -47,7 +47,7 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC @ValueSource(strings = { """ 1|COMPLETED - 2|GATED + 2|GATE_WAITING """, """ # Chunk already queued -> waiting for complete @@ -69,8 +69,9 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC """, """ # Not all steps ready to advance + # Latch Count: 1 1|COMPLETED - 2|READY # a single ready chunk + 2|READY,2|QUEUED # a single ready chunk 2|IN_PROGRESS """, """ @@ -82,32 +83,36 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC 3|READY """, """ - 1|COMPLETED - 2|READY - 2|QUEUED - 2|COMPLETED - 2|ERRORED - 2|FAILED - 2|IN_PROGRESS - 3|GATED - 3|GATED + # when current step is not all queued, should queue READY chunks + # Latch Count: 1 + 1|COMPLETED + 2|READY,2|QUEUED + 2|QUEUED + 2|COMPLETED + 2|ERRORED + 2|FAILED + 2|IN_PROGRESS + 3|GATE_WAITING + 3|QUEUED """, """ - 1|COMPLETED - 2|READY - 2|QUEUED - 2|COMPLETED - 2|ERRORED - 2|FAILED - 2|IN_PROGRESS - 3|QUEUED # a lie - 3|GATED + # when current step is all queued but not done, should not proceed + 1|COMPLETED + 2|COMPLETED + 2|QUEUED + 2|COMPLETED + 2|ERRORED + 2|FAILED + 2|IN_PROGRESS + 3|GATE_WAITING + 3|GATE_WAITING """ }) default void testGatedStep2NotReady_notAdvance(String theChunkState) throws InterruptedException { // setup + int expectedLatchCount = getLatchCountFromState(theChunkState); PointcutLatch sendingLatch = disableWorkChunkMessageHandler(); - sendingLatch.setExpectedCount(0); + sendingLatch.setExpectedCount(expectedLatchCount); JobMaintenanceStateInformation result = setupGatedWorkChunkTransitionTest(theChunkState, true); createChunksInStates(result); @@ -117,11 +122,21 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC // verify // nothing ever queued -> nothing ever sent to queue - verifyWorkChunkMessageHandlerCalled(sendingLatch, 0); + verifyWorkChunkMessageHandlerCalled(sendingLatch, expectedLatchCount); verifyWorkChunkFinalStates(result); } - @Disabled + /** + * Returns the expected latch count specified in the state. Defaults to 0 if not found. + * Expected format: # Latch Count: {} + * e.g. # Latch Count: 3 + */ + private int getLatchCountFromState(String theState){ + String keyStr = "# Latch Count: "; + int index = theState.indexOf(keyStr); + return index == -1 ? 0 : theState.charAt(index + keyStr.length()) - '0'; + } + @ParameterizedTest @ValueSource(strings = { """ @@ -129,27 +144,30 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC 1|COMPLETED 2|COMPLETED 2|COMPLETED - 3|GATED|READY - 3|GATED|READY + 3|GATE_WAITING,3|QUEUED + 3|GATE_WAITING,3|QUEUED """, """ # OLD code only 1|COMPLETED - 2|QUEUED,2|READY - 2|QUEUED,2|READY + 2|COMPLETED + 2|COMPLETED + 3|QUEUED,3|QUEUED + 3|QUEUED,3|QUEUED """, """ - # mixed code only + # mixed code 1|COMPLETED 2|COMPLETED 2|COMPLETED - 3|GATED|READY - 3|QUEUED|READY + 3|GATE_WAITING,3|QUEUED + 3|QUEUED,3|QUEUED """ }) default void testGatedStep2ReadyToAdvance_advanceToStep3(String theChunkState) throws InterruptedException { // setup PointcutLatch sendingLatch = disableWorkChunkMessageHandler(); + sendingLatch.setExpectedCount(2); JobMaintenanceStateInformation result = setupGatedWorkChunkTransitionTest(theChunkState, true); createChunksInStates(result); @@ -157,8 +175,7 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC runMaintenancePass(); // verify - // things are being set to READY; is anything being queued? - verifyWorkChunkMessageHandlerCalled(sendingLatch, 0); + verifyWorkChunkMessageHandlerCalled(sendingLatch, 2); verifyWorkChunkFinalStates(result); } diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkCommon.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkCommon.java index 99ce4bd9fd2..b1e7417635b 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkCommon.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkCommon.java @@ -35,8 +35,8 @@ public interface IWorkChunkCommon extends WorkChunkTestConstants { return getTestManager().createInstance(); } - default String storeWorkChunk(String theJobDefinitionId, String theTargetStepId, String theInstanceId, int theSequence, String theSerializedData) { - return getTestManager().storeWorkChunk(theJobDefinitionId, theTargetStepId, theInstanceId, theSequence, theSerializedData); + default String storeWorkChunk(String theJobDefinitionId, String theTargetStepId, String theInstanceId, int theSequence, String theSerializedData, boolean theGatedExecution) { + return getTestManager().storeWorkChunk(theJobDefinitionId, theTargetStepId, theInstanceId, theSequence, theSerializedData, theGatedExecution); } default void runInTransaction(Runnable theRunnable) { @@ -80,6 +80,10 @@ public interface IWorkChunkCommon extends WorkChunkTestConstants { return getTestManager().createChunk(theJobInstanceId); } + default String createChunk(String theJobInstanceId, boolean theGatedExecution) { + return getTestManager().createChunk(theJobInstanceId, theGatedExecution); + } + /** * Enable/disable the maintenance runner (So it doesn't run on a scheduler) */ diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java index 5be2ce0596d..eb279780b58 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java @@ -6,6 +6,8 @@ import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; import ca.uhn.hapi.fhir.batch2.test.support.JobMaintenanceStateInformation; import ca.uhn.test.concurrency.PointcutLatch; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -15,21 +17,30 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT Logger ourLog = LoggerFactory.getLogger(IWorkChunkStateTransitions.class); - @Test - default void chunkCreation_isQueued() { + @ParameterizedTest + @CsvSource({ + "false, READY", + "true, GATE_WAITING" + }) + default void chunkCreation_isInExpectedStatus(boolean theGatedExecution, WorkChunkStatusEnum expectedStatus) { String jobInstanceId = createAndStoreJobInstance(null); - String myChunkId = createChunk(jobInstanceId); + String myChunkId = createChunk(jobInstanceId, theGatedExecution); WorkChunk fetchedWorkChunk = freshFetchWorkChunk(myChunkId); - assertEquals(WorkChunkStatusEnum.READY, fetchedWorkChunk.getStatus(), "New chunks are READY"); + assertEquals(expectedStatus, fetchedWorkChunk.getStatus(), "New chunks are " + expectedStatus); } - @Test - default void chunkReceived_queuedToInProgress() throws InterruptedException { + @ParameterizedTest + @CsvSource({ + "false", + "true" + }) + default void chunkReceived_queuedToInProgress(boolean theGatedExecution) throws InterruptedException { PointcutLatch sendLatch = disableWorkChunkMessageHandler(); sendLatch.setExpectedCount(1); - String jobInstanceId = createAndStoreJobInstance(null); - String myChunkId = createChunk(jobInstanceId); + + String jobInstanceId = createAndStoreJobInstance(withJobDefinition(theGatedExecution)); + String myChunkId = createChunk(jobInstanceId, theGatedExecution); runMaintenancePass(); // the worker has received the chunk, and marks it started. diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java index 80119ffbdf3..602708fb189 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java @@ -2,18 +2,17 @@ package ca.uhn.hapi.fhir.batch2.test; import ca.uhn.fhir.batch2.model.JobDefinition; import ca.uhn.fhir.batch2.model.JobInstance; -import ca.uhn.fhir.batch2.model.JobWorkNotification; import ca.uhn.fhir.batch2.model.WorkChunk; import ca.uhn.fhir.batch2.model.WorkChunkCompletionEvent; -import ca.uhn.fhir.batch2.model.WorkChunkCreateEvent; import ca.uhn.fhir.batch2.model.WorkChunkErrorEvent; import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; import ca.uhn.hapi.fhir.batch2.test.support.JobMaintenanceStateInformation; import ca.uhn.test.concurrency.PointcutLatch; import com.google.common.collect.ImmutableList; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; -import java.util.ArrayList; import java.util.Date; import java.util.Iterator; import java.util.List; @@ -23,8 +22,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.doNothing; public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestConstants { @@ -33,7 +30,7 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC JobInstance instance = createInstance(); String instanceId = getSvc().storeNewInstance(instance); - String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null); + String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null, false); runInTransaction(() -> { WorkChunk chunk = freshFetchWorkChunk(id); @@ -41,17 +38,21 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC }); } - @Test - default void testWorkChunkCreate_inReadyState() { + @ParameterizedTest + @CsvSource({ + "false, READY", + "true, GATE_WAITING" + }) + default void testWorkChunkCreate_inExpectedStatus(boolean theGatedExecution, WorkChunkStatusEnum expectedStatus) { JobInstance instance = createInstance(); String instanceId = getSvc().storeNewInstance(instance); enableMaintenanceRunner(false); - String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA); + String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA, theGatedExecution); assertNotNull(id); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, freshFetchWorkChunk(id).getStatus())); + runInTransaction(() -> assertEquals(expectedStatus, freshFetchWorkChunk(id).getStatus())); } @Test diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/support/JobMaintenanceStateInformation.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/support/JobMaintenanceStateInformation.java index bee8f43c05c..511f675c2e6 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/support/JobMaintenanceStateInformation.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/support/JobMaintenanceStateInformation.java @@ -145,7 +145,8 @@ public class JobMaintenanceStateInformation { if (jobDef.isGatedExecution()) { AtomicReference latestStepId = new AtomicReference<>(); int totalSteps = jobDef.getSteps().size(); - for (int i = totalSteps - 1; i >= 0; i--) { + // ignore the last step + for (int i = totalSteps - 2; i >= 0; i--) { JobDefinitionStep step = jobDef.getSteps().get(i); if (stepIds.contains(step.getStepId())) { latestStepId.set(step.getStepId()); diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java index d40495f3d40..828aee61225 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java @@ -280,4 +280,28 @@ public interface IJobPersistence extends IWorkChunkPersistence { @VisibleForTesting WorkChunk createWorkChunk(WorkChunk theWorkChunk); + + /** + * Atomically advance the given job to the given step and change the status of all chunks in the next step to READY + * @param theJobInstanceId the id of the job instance to be updated + * @param theNextStepId the id of the next job step + * @return + */ + @Transactional(propagation = Propagation.REQUIRES_NEW) + boolean advanceJobStepAndUpdateChunkStatus(String theJobInstanceId, String theNextStepId); + + /** + * Update all chunks of the given step id for the given job from + * @param theJobInstanceId the id of the job instance to be updated + * @param theStepId the id of the step which the chunks belong to + * @param theNewStatus the new status + * @param theOldStatus the old status + * @return + */ + @Transactional(propagation = Propagation.REQUIRES_NEW) + int updateAllChunksForStepWithStatus( + String theJobInstanceId, + String theStepId, + WorkChunkStatusEnum theNewStatus, + WorkChunkStatusEnum theOldStatus); } diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IWorkChunkPersistence.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IWorkChunkPersistence.java index 0bde5e4d524..b52e045824c 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IWorkChunkPersistence.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IWorkChunkPersistence.java @@ -55,7 +55,8 @@ public interface IWorkChunkPersistence { * The first state event, as the chunk is created. * This method should be atomic and should only * return when the chunk has been successfully stored in the database. - * Chunk should be stored with a status of {@link WorkChunkStatusEnum#QUEUED} + * Chunk should be stored with a status of {@link WorkChunkStatusEnum#READY} or + * {@link WorkChunkStatusEnum#GATE_WAITING} for ungated and gated jobs, respectively. * * @param theBatchWorkChunk the batch work chunk to be stored * @return a globally unique identifier for this chunk. diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/JobDataSink.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/JobDataSink.java index d7b00bd32d7..0c067ef7a7c 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/JobDataSink.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/JobDataSink.java @@ -82,7 +82,13 @@ class JobDataSink still proceed + if (workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.GATE_WAITING)) && theWorkCursor.isFirstStep()) { + // We are in the first step and all chunks are in GATE_WAITING + // this means that the job has just started, no workchunks have been queued yet -> proceed. return true; } - if (workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.READY))) { + if (workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.COMPLETED))) { + // all workchunks for the current step are in COMPLETED -> proceed. + return true; + } + + // all workchunks for gated jobs should be turned to QUEUED immediately after they are set to READY + // but in case we die in between, this conditional ought to catch the READY chunks. + if (workChunkStatuses.contains(WorkChunkStatusEnum.READY)) { if (theWorkCursor.isFirstStep()) { - // first step - all ready means we're ready to proceed to the next step + // first step - all ready means we're ready to proceed to enqueue return true; } else { // it's a future step; @@ -300,11 +307,11 @@ public class JobInstanceProcessor { * * We could block chunks from being moved from QUEUE'd to READY here for gated steps * but currently, progress is calculated by looking at completed chunks only; - * we'd need a new GATE_WAITING state to move chunks to to prevent jobs from + * we'd need a new GATE_WAITING state to move chunks to prevent jobs from * completing prematurely. */ private void enqueueReadyChunks( - JobInstance theJobInstance, JobDefinition theJobDefinition, boolean theIsGatedExecutionAdvancementBool) { + JobInstance theJobInstance, JobDefinition theJobDefinition, boolean theIsGatedExecutionBool) { Iterator iter = getReadyChunks(); AtomicInteger counter = new AtomicInteger(); @@ -315,8 +322,7 @@ public class JobInstanceProcessor { return JobWorkCursor.fromJobDefinitionAndRequestedStepId(theJobDefinition, metadata.getTargetStepId()); }); counter.getAndIncrement(); - if (!theIsGatedExecutionAdvancementBool - && (theJobDefinition.isGatedExecution() || jobWorkCursor.isReductionStep())) { + if (!theIsGatedExecutionBool && (theJobDefinition.isGatedExecution() || jobWorkCursor.isReductionStep())) { /* * Gated executions are queued later when all work chunks are ready. * @@ -398,32 +404,36 @@ public class JobInstanceProcessor { readyChunksForNextStep.size()); } - // TODO - // create a new persistence transition for state advance - // update stepId to next step AND update all chunks in this step to READY (from GATED or QUEUED ;-P) - // so we can queue them safely. + boolean isEnqueue; + String currentStepId = theWorkCursor.getCurrentStepId(); + Set workChunkStatusesForCurrentStep = + myJobPersistence.getDistinctWorkChunkStatesForJobAndStep(theInstance.getInstanceId(), currentStepId); + if (workChunkStatusesForCurrentStep.equals(Set.of(WorkChunkStatusEnum.GATE_WAITING)) + && theWorkCursor.isFirstStep()) { + // this means that the job has just started, no workchunks have been queued yet + // turn the first chunk to READY, do NOT advance the step. + isEnqueue = myJobPersistence.updateAllChunksForStepWithStatus( + instanceId, currentStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.GATE_WAITING) + != 0; + } else if (workChunkStatusesForCurrentStep.equals(Set.of(WorkChunkStatusEnum.COMPLETED))) { + // update the job step so the workers will process them. + // if it's the last (gated) step, there will be no change - but we should + // queue up the chunks anyway + isEnqueue = theWorkCursor.isFinalStep() + || myJobPersistence.advanceJobStepAndUpdateChunkStatus(instanceId, nextStepId); + } else { + // this means that the current step's chunks contains only READY and QUEUED chunks, possibly leftover from + // other maintenance job who died in the middle + // should enqueue the rest of the ready chunks + isEnqueue = true; + } - // update the job step so the workers will process them. - // if it's the last (gated) step, there will be no change - but we should - // queue up the chunks anyways - boolean changed = theWorkCursor.isFinalStep() - || myJobPersistence.updateInstance(instanceId, instance -> { - if (instance.getCurrentGatedStepId().equals(nextStepId)) { - // someone else beat us here. No changes - return false; - } - instance.setCurrentGatedStepId(nextStepId); - return true; - }); - - if (!changed) { + if (!isEnqueue) { // we collided with another maintenance job. ourLog.warn("Skipping gate advance to {} for instance {} - already advanced.", nextStepId, instanceId); return; } - ourLog.debug("Moving gated instance {} to next step.", theInstance.getInstanceId()); - // because we now have all gated job chunks in READY state, // we can enqueue them enqueueReadyChunks(theInstance, theJobDefinition, true); diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkCreateEvent.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkCreateEvent.java index 95e07c87761..00459780af0 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkCreateEvent.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkCreateEvent.java @@ -36,6 +36,7 @@ public class WorkChunkCreateEvent { public final String instanceId; public final int sequence; public final String serializedData; + public final boolean isGatedExecution; /** * Constructor @@ -52,20 +53,24 @@ public class WorkChunkCreateEvent { @Nonnull String theTargetStepId, @Nonnull String theInstanceId, int theSequence, - @Nullable String theSerializedData) { + @Nullable String theSerializedData, + boolean theGatedExecution) { jobDefinitionId = theJobDefinitionId; jobDefinitionVersion = theJobDefinitionVersion; targetStepId = theTargetStepId; instanceId = theInstanceId; sequence = theSequence; serializedData = theSerializedData; + isGatedExecution = theGatedExecution; } public static WorkChunkCreateEvent firstChunk(JobDefinition theJobDefinition, String theInstanceId) { String firstStepId = theJobDefinition.getFirstStepId(); String jobDefinitionId = theJobDefinition.getJobDefinitionId(); int jobDefinitionVersion = theJobDefinition.getJobDefinitionVersion(); - return new WorkChunkCreateEvent(jobDefinitionId, jobDefinitionVersion, firstStepId, theInstanceId, 0, null); + boolean isGatedExecution = theJobDefinition.isGatedExecution(); + return new WorkChunkCreateEvent( + jobDefinitionId, jobDefinitionVersion, firstStepId, theInstanceId, 0, null, isGatedExecution); } @Override @@ -83,6 +88,7 @@ public class WorkChunkCreateEvent { .append(instanceId, that.instanceId) .append(sequence, that.sequence) .append(serializedData, that.serializedData) + .append(isGatedExecution, that.isGatedExecution) .isEquals(); } @@ -95,6 +101,7 @@ public class WorkChunkCreateEvent { .append(instanceId) .append(sequence) .append(serializedData) + .append(isGatedExecution) .toHashCode(); } } diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkStatusEnum.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkStatusEnum.java index 7eb6d4f41b0..9b9d2517671 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkStatusEnum.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkStatusEnum.java @@ -32,6 +32,7 @@ import java.util.Set; */ public enum WorkChunkStatusEnum { // wipmb For 6.8 Add WAITING for gated, and READY for in db, but not yet sent to channel. + GATE_WAITING, READY, QUEUED, IN_PROGRESS, @@ -59,6 +60,8 @@ public enum WorkChunkStatusEnum { public Set getNextStates() { switch (this) { + case GATE_WAITING: + return EnumSet.of(READY); case READY: return EnumSet.of(QUEUED); case QUEUED: diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/progress/InstanceProgress.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/progress/InstanceProgress.java index b284f16cb96..7228ed65038 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/progress/InstanceProgress.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/progress/InstanceProgress.java @@ -73,6 +73,7 @@ public class InstanceProgress { statusToCountMap.put(theChunk.getStatus(), statusToCountMap.getOrDefault(theChunk.getStatus(), 0) + 1); switch (theChunk.getStatus()) { + case GATE_WAITING: case READY: case QUEUED: case IN_PROGRESS: From 0aabdbdf4f1d501bd482dd699b4e3dae5353d1eb Mon Sep 17 00:00:00 2001 From: tyner Date: Wed, 10 Apr 2024 05:41:36 -0400 Subject: [PATCH 02/13] - fix merges conflicts - set first chunk to be always created in READY --- .../java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java | 4 +++- .../hapi/fhir/batch2/test/IWorkChunkStateTransitions.java | 6 ++---- .../java/ca/uhn/fhir/batch2/model/WorkChunkCreateEvent.java | 6 +++++- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java index 4425b062c6b..8263ac0eb98 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java @@ -37,7 +37,7 @@ public interface ITestFixture { WorkChunk freshFetchWorkChunk(String theChunkId); - String storeWorkChunk(String theJobDefinitionId, String theTargetStepId, String theInstanceId, int theSequence, String theSerializedData); + String storeWorkChunk(String theJobDefinitionId, String theTargetStepId, String theInstanceId, int theSequence, String theSerializedData, boolean theGatedExecution); void runInTransaction(Runnable theRunnable); @@ -62,6 +62,8 @@ public interface ITestFixture { */ String createChunk(String theJobInstanceId); + String createChunk(String theJobInstanceId, boolean theGatedExecution); + /** * Enable/disable the maintenance runner (So it doesn't run on a scheduler) */ diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java index e826b2af6bd..084b8bd0e2a 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java @@ -19,9 +19,6 @@ */ package ca.uhn.hapi.fhir.batch2.test; -import ca.uhn.fhir.batch2.model.WorkChunk; -import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; - import ca.uhn.fhir.batch2.model.JobDefinition; import ca.uhn.fhir.batch2.model.WorkChunk; import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; @@ -61,7 +58,8 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT PointcutLatch sendLatch = getTestManager().disableWorkChunkMessageHandler(); sendLatch.setExpectedCount(1); - String jobInstanceId = getTestManager().createAndStoreJobInstance(withJobDefinition(theGatedExecution)); + JobDefinition jobDef = getTestManager().withJobDefinition(false); + String jobInstanceId = getTestManager().createAndStoreJobInstance(jobDef); String myChunkId = getTestManager().createChunk(jobInstanceId, theGatedExecution); getTestManager().runMaintenancePass(); diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkCreateEvent.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkCreateEvent.java index 00459780af0..c2b489016b7 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkCreateEvent.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkCreateEvent.java @@ -64,11 +64,15 @@ public class WorkChunkCreateEvent { isGatedExecution = theGatedExecution; } + /** + * Creates the WorkChunkCreateEvent for the first chunk of a job. + */ public static WorkChunkCreateEvent firstChunk(JobDefinition theJobDefinition, String theInstanceId) { String firstStepId = theJobDefinition.getFirstStepId(); String jobDefinitionId = theJobDefinition.getJobDefinitionId(); int jobDefinitionVersion = theJobDefinition.getJobDefinitionVersion(); - boolean isGatedExecution = theJobDefinition.isGatedExecution(); + // the first chunk of a job is always READY, no matter whether the job is gated + boolean isGatedExecution = false; return new WorkChunkCreateEvent( jobDefinitionId, jobDefinitionVersion, firstStepId, theInstanceId, 0, null, isGatedExecution); } From 1c1ceb0b27be9f6cdf02054ad11ae4aa287d2dad Mon Sep 17 00:00:00 2001 From: tyner Date: Wed, 10 Apr 2024 07:51:04 -0400 Subject: [PATCH 03/13] - have only one path through the equeueReady method - fixed tests --- .../jpa/batch2/JpaJobPersistenceImplTest.java | 125 ++++++++++-------- ...tractIJobPersistenceSpecificationTest.java | 23 +++- .../batch2/test/IJobMaintenanceActions.java | 4 +- .../hapi/fhir/batch2/test/ITestFixture.java | 4 + .../test/IWorkChunkStateTransitions.java | 62 +++++++-- .../batch2/test/IWorkChunkStorageTests.java | 7 +- .../batch2/test/WorkChunkTestConstants.java | 4 +- .../maintenance/JobInstanceProcessor.java | 119 ++--------------- 8 files changed, 166 insertions(+), 182 deletions(-) diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index f2da87cd00f..d872def6b83 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -132,13 +132,18 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { return mySvc.onWorkChunkCreate(batchWorkChunk); } + private String storeFirstWorkChunk(String theJobDefinitionId, String theTargetStepId, String theInstanceId, int theSequence, String theSerializedData) { + WorkChunkCreateEvent batchWorkChunk = new WorkChunkCreateEvent(theJobDefinitionId, TestJobDefinitionUtils.TEST_JOB_VERSION, theTargetStepId, theInstanceId, theSequence, theSerializedData, false); + return mySvc.onWorkChunkCreate(batchWorkChunk); + } + @Test public void testStoreAndFetchInstance() { JobInstance instance = createInstance(); String instanceId = mySvc.storeNewInstance(instance); runInTransaction(() -> { - Batch2JobInstanceEntity instanceEntity = myJobInstanceRepository.findById(instanceId).orElseThrow(IllegalStateException::new); + Batch2JobInstanceEntity instanceEntity = findInstanceByIdOrThrow(instanceId); assertEquals(StatusEnum.QUEUED, instanceEntity.getStatus()); }); @@ -151,7 +156,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { assertEquals(instance.getReport(), foundInstance.getReport()); runInTransaction(() -> { - Batch2JobInstanceEntity instanceEntity = myJobInstanceRepository.findById(instanceId).orElseThrow(IllegalStateException::new); + Batch2JobInstanceEntity instanceEntity = findInstanceByIdOrThrow(instanceId); assertEquals(StatusEnum.QUEUED, instanceEntity.getStatus()); }); } @@ -372,7 +377,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { JobInstance instance = createInstance(true, false); String instanceId = mySvc.storeNewInstance(instance); - Date updateTime = runInTransaction(() -> new Date(myJobInstanceRepository.findById(instanceId).orElseThrow().getUpdateTime().getTime())); + Date updateTime = runInTransaction(() -> new Date(findInstanceByIdOrThrow(instanceId).getUpdateTime().getTime())); sleepUntilTimeChange(); @@ -380,7 +385,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { runInTransaction(() -> mySvc.updateInstanceUpdateTime(instanceId)); // Verify - Date updateTime2 = runInTransaction(() -> new Date(myJobInstanceRepository.findById(instanceId).orElseThrow().getUpdateTime().getTime())); + Date updateTime2 = runInTransaction(() -> new Date(findInstanceByIdOrThrow(instanceId).getUpdateTime().getTime())); assertNotEquals(updateTime, updateTime2); } @@ -389,20 +394,20 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { // setup JobInstance instance = createInstance(true, true); String instanceId = mySvc.storeNewInstance(instance); - String chunkIdFirstStep = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null, true); String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); - runInTransaction(() -> assertEquals(TARGET_STEP_ID, myJobInstanceRepository.findById(instanceId).orElseThrow(IllegalArgumentException::new).getCurrentGatedStepId())); + runInTransaction(() -> assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); // execute runInTransaction(() -> mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, LAST_STEP_ID)); // verify - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(chunkIdFirstStep).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkIdSecondStep1).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkIdSecondStep2).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(LAST_STEP_ID, myJobInstanceRepository.findById(instanceId).orElseThrow(IllegalArgumentException::new).getCurrentGatedStepId())); + runInTransaction(() -> { + assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); + assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); + assertEquals(LAST_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId()); + }); } @Test @@ -416,17 +421,21 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); - runInTransaction(() -> assertEquals(expectedNoChangeStatus, myWorkChunkRepository.findById(chunkIdFirstStep).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(expectedNoChangeStatus, myWorkChunkRepository.findById(chunkIdSecondStep1).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(expectedNoChangeStatus, myWorkChunkRepository.findById(chunkIdSecondStep2).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> { + assertEquals(expectedNoChangeStatus, findChunkByIdOrThrow(chunkIdFirstStep).getStatus()); + assertEquals(expectedNoChangeStatus, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); + assertEquals(expectedNoChangeStatus, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); + }); // execute runInTransaction(() -> mySvc.updateAllChunksForStepWithStatus(instanceId, LAST_STEP_ID, expectedChangedStatus, WorkChunkStatusEnum.GATE_WAITING)); // verify - runInTransaction(() -> assertEquals(expectedNoChangeStatus, myWorkChunkRepository.findById(chunkIdFirstStep).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(expectedChangedStatus, myWorkChunkRepository.findById(chunkIdSecondStep1).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(expectedChangedStatus, myWorkChunkRepository.findById(chunkIdSecondStep2).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> { + assertEquals(expectedNoChangeStatus, findChunkByIdOrThrow(chunkIdFirstStep).getStatus()); + assertEquals(expectedChangedStatus, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); + assertEquals(expectedChangedStatus, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); + }); } @Test @@ -445,15 +454,10 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String instanceId = mySvc.storeNewInstance(instance); // execute & verify - String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null, theGatedExecution); - runInTransaction(() -> assertEquals(theExpectedStatusOnCreate, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + String id = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, theGatedExecution); + runInTransaction(() -> assertEquals(theExpectedStatusOnCreate, findChunkByIdOrThrow(id).getStatus())); myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(theExpectedStatusAfterTransition, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); - - if (WorkChunkStatusEnum.READY.equals(theExpectedStatusAfterTransition)) { - myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); - } + runInTransaction(() -> assertEquals(theExpectedStatusAfterTransition, findChunkByIdOrThrow(id).getStatus())); WorkChunk chunk = mySvc.onWorkChunkDequeue(id).orElseThrow(IllegalArgumentException::new); assertNull(chunk.getData()); @@ -469,30 +473,38 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String instanceId = mySvc.storeNewInstance(instance); // execute & verify - String firstChunkId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, expectedFirstChunkData, isGatedExecution); + String firstChunkId = storeFirstWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, expectedFirstChunkData); String secondChunkId = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, expectedSecondChunkData, isGatedExecution); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> { + assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(firstChunkId).getStatus()); + assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(secondChunkId).getStatus()); + }); myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> { + assertEquals(WorkChunkStatusEnum.QUEUED, findChunkByIdOrThrow(firstChunkId).getStatus()); + assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(secondChunkId).getStatus()); + }); WorkChunk actualFirstChunkData = mySvc.onWorkChunkDequeue(firstChunkId).orElseThrow(IllegalArgumentException::new); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, findChunkByIdOrThrow(firstChunkId).getStatus())); assertEquals(expectedFirstChunkData, actualFirstChunkData.getData()); mySvc.onWorkChunkCompletion(new WorkChunkCompletionEvent(firstChunkId, 50, 0)); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.COMPLETED, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.GATE_WAITING, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> { + assertEquals(WorkChunkStatusEnum.COMPLETED, findChunkByIdOrThrow(firstChunkId).getStatus()); + assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(secondChunkId).getStatus()); + }); myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.COMPLETED, myWorkChunkRepository.findById(firstChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> { + assertEquals(WorkChunkStatusEnum.COMPLETED, findChunkByIdOrThrow(firstChunkId).getStatus()); + assertEquals(WorkChunkStatusEnum.QUEUED, findChunkByIdOrThrow(secondChunkId).getStatus()); + }); WorkChunk actualSecondChunkData = mySvc.onWorkChunkDequeue(secondChunkId).orElseThrow(IllegalArgumentException::new); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, findChunkByIdOrThrow(secondChunkId).getStatus())); assertEquals(expectedSecondChunkData, actualSecondChunkData.getData()); } @@ -572,16 +584,11 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { JobInstance instance = createInstance(true, theGatedExecution); String instanceId = mySvc.storeNewInstance(instance); - String id = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA, theGatedExecution); + String id = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, CHUNK_DATA, theGatedExecution); assertNotNull(id); - runInTransaction(() -> assertEquals(theExpectedCreatedStatus, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(theExpectedCreatedStatus, findChunkByIdOrThrow(id).getStatus())); myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(theExpectedTransitionStatus, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); - - if (WorkChunkStatusEnum.READY.equals(theExpectedTransitionStatus)){ - myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); - } + runInTransaction(() -> assertEquals(theExpectedTransitionStatus, findChunkByIdOrThrow(id).getStatus())); WorkChunk chunk = mySvc.onWorkChunkDequeue(id).orElseThrow(IllegalArgumentException::new); assertEquals(36, chunk.getInstanceId().length()); @@ -590,7 +597,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { assertEquals(WorkChunkStatusEnum.IN_PROGRESS, chunk.getStatus()); assertEquals(CHUNK_DATA, chunk.getData()); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, myWorkChunkRepository.findById(id).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, findChunkByIdOrThrow(id).getStatus())); } @Test @@ -601,9 +608,9 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String chunkId = storeWorkChunk(DEF_CHUNK_ID, STEP_CHUNK_ID, instanceId, SEQUENCE_NUMBER, CHUNK_DATA, isGatedExecution); assertNotNull(chunkId); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(chunkId).getStatus())); myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, findChunkByIdOrThrow(chunkId).getStatus())); WorkChunk chunk = mySvc.onWorkChunkDequeue(chunkId).orElseThrow(IllegalArgumentException::new); assertEquals(SEQUENCE_NUMBER, chunk.getSequence()); @@ -613,13 +620,13 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { assertNull(chunk.getEndTime()); assertNull(chunk.getRecordsProcessed()); assertNotNull(chunk.getData()); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.IN_PROGRESS, findChunkByIdOrThrow(chunkId).getStatus())); sleepUntilTimeChange(); mySvc.onWorkChunkCompletion(new WorkChunkCompletionEvent(chunkId, 50, 0)); runInTransaction(() -> { - Batch2WorkChunkEntity entity = myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new); + Batch2WorkChunkEntity entity = findChunkByIdOrThrow(chunkId); assertEquals(WorkChunkStatusEnum.COMPLETED, entity.getStatus()); assertEquals(50, entity.getRecordsProcessed()); assertNotNull(entity.getCreateTime()); @@ -639,9 +646,9 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String chunkId = storeWorkChunk(JOB_DEFINITION_ID, TestJobDefinitionUtils.FIRST_STEP_ID, instanceId, SEQUENCE_NUMBER, null, isGatedExecution); assertNotNull(chunkId); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(chunkId).getStatus())); myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, findChunkByIdOrThrow(chunkId).getStatus())); WorkChunk chunk = mySvc.onWorkChunkDequeue(chunkId).orElseThrow(IllegalArgumentException::new); assertEquals(SEQUENCE_NUMBER, chunk.getSequence()); @@ -652,7 +659,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { WorkChunkErrorEvent request = new WorkChunkErrorEvent(chunkId).setErrorMsg("This is an error message"); mySvc.onWorkChunkError(request); runInTransaction(() -> { - Batch2WorkChunkEntity entity = myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new); + Batch2WorkChunkEntity entity = findChunkByIdOrThrow(chunkId); assertEquals(WorkChunkStatusEnum.ERRORED, entity.getStatus()); assertEquals("This is an error message", entity.getErrorMessage()); assertNotNull(entity.getCreateTime()); @@ -668,7 +675,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { WorkChunkErrorEvent request2 = new WorkChunkErrorEvent(chunkId).setErrorMsg("This is an error message 2"); mySvc.onWorkChunkError(request2); runInTransaction(() -> { - Batch2WorkChunkEntity entity = myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new); + Batch2WorkChunkEntity entity = findChunkByIdOrThrow(chunkId); assertEquals(WorkChunkStatusEnum.ERRORED, entity.getStatus()); assertEquals("This is an error message 2", entity.getErrorMessage()); assertNotNull(entity.getCreateTime()); @@ -692,9 +699,9 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String chunkId = storeWorkChunk(DEF_CHUNK_ID, STEP_CHUNK_ID, instanceId, SEQUENCE_NUMBER, null, isGatedExecution); assertNotNull(chunkId); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(chunkId).getStatus())); myBatch2JobHelper.runMaintenancePass(); - runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new).getStatus())); + runInTransaction(() -> assertEquals(WorkChunkStatusEnum.QUEUED, findChunkByIdOrThrow(chunkId).getStatus())); WorkChunk chunk = mySvc.onWorkChunkDequeue(chunkId).orElseThrow(IllegalArgumentException::new); assertEquals(SEQUENCE_NUMBER, chunk.getSequence()); @@ -704,7 +711,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { mySvc.onWorkChunkFailed(chunkId, "This is an error message"); runInTransaction(() -> { - Batch2WorkChunkEntity entity = myWorkChunkRepository.findById(chunkId).orElseThrow(IllegalArgumentException::new); + Batch2WorkChunkEntity entity = findChunkByIdOrThrow(chunkId); assertEquals(WorkChunkStatusEnum.FAILED, entity.getStatus()); assertEquals("This is an error message", entity.getErrorMessage()); assertNotNull(entity.getCreateTime()); @@ -863,4 +870,12 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { Arguments.of(WorkChunkStatusEnum.COMPLETED, false) ); } + + private Batch2JobInstanceEntity findInstanceByIdOrThrow(String instanceId) { + return myJobInstanceRepository.findById(instanceId).orElseThrow(IllegalStateException::new); + } + + private Batch2WorkChunkEntity findChunkByIdOrThrow(String secondChunkId) { + return myWorkChunkRepository.findById(secondChunkId).orElseThrow(IllegalArgumentException::new); + } } diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java index 53608062525..0d2a00bc622 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java @@ -98,9 +98,9 @@ public abstract class AbstractIJobPersistenceSpecificationTest implements IJobMa .setJobDefinitionVersion(JOB_DEF_VER) .setJobDescription("A job description") .setParametersType(TestJobParameters.class) - .addFirstStep(TARGET_STEP_ID, "the first step", TestJobStep2InputType.class, (theStepExecutionDetails, theDataSink) -> new RunOutcome(0)) - .addIntermediateStep("2nd-step-id", "the second step", TestJobStep3InputType.class, (theStepExecutionDetails, theDataSink) -> new RunOutcome(0)) - .addLastStep("last-step-id", "the final step", (theStepExecutionDetails, theDataSink) -> new RunOutcome(0)); + .addFirstStep(FIRST_STEP_ID, "the first step", TestJobStep2InputType.class, (theStepExecutionDetails, theDataSink) -> new RunOutcome(0)) + .addIntermediateStep(SECOND_STEP_ID, "the second step", TestJobStep3InputType.class, (theStepExecutionDetails, theDataSink) -> new RunOutcome(0)) + .addLastStep(LAST_STEP_ID, "the final step", (theStepExecutionDetails, theDataSink) -> new RunOutcome(0)); if (theIsGatedBoolean) { builder.gatedExecution(); } @@ -177,6 +177,11 @@ public abstract class AbstractIJobPersistenceSpecificationTest implements IJobMa return mySvc.onWorkChunkCreate(batchWorkChunk); } + public String storeFirstWorkChunk(JobDefinition theJobDefinition, String theInstanceId) { + WorkChunkCreateEvent batchWorkChunk = WorkChunkCreateEvent.firstChunk(theJobDefinition, theInstanceId); + return mySvc.onWorkChunkCreate(batchWorkChunk); + } + public abstract PlatformTransactionManager getTxManager(); public JobInstance freshFetchJobInstance(String theInstanceId) { @@ -233,11 +238,19 @@ public abstract class AbstractIJobPersistenceSpecificationTest implements IJobMa } public String createChunk(String theInstanceId) { - return storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, theInstanceId, 0, CHUNK_DATA, false); + return storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, theInstanceId, 0, CHUNK_DATA, false); } public String createChunk(String theInstanceId, boolean theGatedExecution) { - return storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, theInstanceId, 0, CHUNK_DATA, theGatedExecution); + return storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, theInstanceId, 0, CHUNK_DATA, theGatedExecution); + } + + public String createChunkInStep(String theInstanceId, String theStepId, boolean theGatedExecution) { + return storeWorkChunk(JOB_DEFINITION_ID, theStepId, theInstanceId, 0, CHUNK_DATA, theGatedExecution); + } + + public String createFirstChunk(JobDefinition theJobDefinition, String theJobInstanceId){ + return storeFirstWorkChunk(theJobDefinition, theJobInstanceId); } public void enableMaintenanceRunner(boolean theToEnable) { diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java index 1fa09807103..843ddecb7c8 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java @@ -79,8 +79,8 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC 1|COMPLETED 2|COMPLETED 2|IN_PROGRESS - 3|READY - 3|READY + 3|GATE_WAITING + 3|GATE_WAITING """, """ # when current step is not all queued, should queue READY chunks diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java index 8263ac0eb98..47a4230728a 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java @@ -64,6 +64,10 @@ public interface ITestFixture { String createChunk(String theJobInstanceId, boolean theGatedExecution); + String createChunkInStep(String theJobInstanceId, String theStepId, boolean theGatedExecution); + + String createFirstChunk(JobDefinition theJobDefinition, String theJobInstanceId); + /** * Enable/disable the maintenance runner (So it doesn't run on a scheduler) */ diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java index 084b8bd0e2a..5d9444e9664 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java @@ -21,15 +21,20 @@ package ca.uhn.hapi.fhir.batch2.test; import ca.uhn.fhir.batch2.model.JobDefinition; import ca.uhn.fhir.batch2.model.WorkChunk; +import ca.uhn.fhir.batch2.model.WorkChunkCompletionEvent; import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; import ca.uhn.hapi.fhir.batch2.test.support.JobMaintenanceStateInformation; +import ca.uhn.hapi.fhir.batch2.test.support.TestJobParameters; import ca.uhn.test.concurrency.PointcutLatch; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.ArrayList; + import static org.junit.jupiter.api.Assertions.assertEquals; public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkTestConstants { @@ -41,7 +46,7 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT "false, READY", "true, GATE_WAITING" }) - default void chunkCreation_isInExpectedStatus(boolean theGatedExecution, WorkChunkStatusEnum expectedStatus) { + default void chunkCreation_nonFirstChunk_isInExpectedStatus(boolean theGatedExecution, WorkChunkStatusEnum expectedStatus) { String jobInstanceId = getTestManager().createAndStoreJobInstance(null); String myChunkId = getTestManager().createChunk(jobInstanceId, theGatedExecution); @@ -50,17 +55,24 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT } @ParameterizedTest - @CsvSource({ - "false", - "true" - }) - default void chunkReceived_queuedToInProgress(boolean theGatedExecution) throws InterruptedException { + @ValueSource(booleans = {true, false}) + default void chunkCreation_firstChunk_isInReady(boolean theGatedExecution) { + JobDefinition jobDef = getTestManager().withJobDefinition(theGatedExecution); + String jobInstanceId = getTestManager().createAndStoreJobInstance(jobDef); + String myChunkId = getTestManager().createFirstChunk(jobDef, jobInstanceId); + + WorkChunk fetchedWorkChunk = getTestManager().freshFetchWorkChunk(myChunkId); + assertEquals(WorkChunkStatusEnum.READY, fetchedWorkChunk.getStatus(), "New chunks are " + WorkChunkStatusEnum.READY); + } + + @Test + default void chunkReceived_forNongatedJob_queuedToInProgress() throws InterruptedException { PointcutLatch sendLatch = getTestManager().disableWorkChunkMessageHandler(); sendLatch.setExpectedCount(1); - JobDefinition jobDef = getTestManager().withJobDefinition(false); + JobDefinition jobDef = getTestManager().withJobDefinition(false); String jobInstanceId = getTestManager().createAndStoreJobInstance(jobDef); - String myChunkId = getTestManager().createChunk(jobInstanceId, theGatedExecution); + String myChunkId = getTestManager().createChunk(jobInstanceId, false); getTestManager().runMaintenancePass(); // the worker has received the chunk, and marks it started. @@ -75,6 +87,40 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT getTestManager().verifyWorkChunkMessageHandlerCalled(sendLatch, 1); } + @Test + default void chunkReceived_forGatedJob_queuedToInProgress() throws InterruptedException { + PointcutLatch sendLatch = getTestManager().disableWorkChunkMessageHandler(); + sendLatch.setExpectedCount(2); + + JobDefinition jobDef = getTestManager().withJobDefinition(true); + String jobInstanceId = getTestManager().createAndStoreJobInstance(jobDef); + String chunkInStep1 = getTestManager().createFirstChunk(jobDef, jobInstanceId); + String chunkInStep2 = getTestManager().createChunkInStep(jobInstanceId, SECOND_STEP_ID, true); + + // dequeue and completes the first chunk + getTestManager().runMaintenancePass(); + // the worker has received the chunk, and marks it started. + WorkChunk chunk1 = getTestManager().getSvc().onWorkChunkDequeue(chunkInStep1).orElseThrow(IllegalArgumentException::new); + assertEquals(WorkChunkStatusEnum.IN_PROGRESS, chunk1.getStatus()); + WorkChunk fetchedWorkChunk = getTestManager().freshFetchWorkChunk(chunkInStep1); + assertEquals(WorkChunkStatusEnum.IN_PROGRESS, fetchedWorkChunk.getStatus()); + + getTestManager().getSvc().onWorkChunkCompletion(new WorkChunkCompletionEvent(chunkInStep1, 50, 0)); + fetchedWorkChunk = getTestManager().freshFetchWorkChunk(chunkInStep1); + assertEquals(WorkChunkStatusEnum.COMPLETED, fetchedWorkChunk.getStatus()); + + // dequeue the second chunk + getTestManager().runMaintenancePass(); + WorkChunk chunk2 = getTestManager().getSvc().onWorkChunkDequeue(chunkInStep2).orElseThrow(IllegalArgumentException::new); + assertEquals(WorkChunkStatusEnum.IN_PROGRESS, chunk2.getStatus()); + assertEquals(CHUNK_DATA, chunk2.getData()); + + WorkChunk fetchedWorkChunk2 = getTestManager().freshFetchWorkChunk(chunkInStep2); + assertEquals(WorkChunkStatusEnum.IN_PROGRESS, fetchedWorkChunk2.getStatus()); + + getTestManager().verifyWorkChunkMessageHandlerCalled(sendLatch, 2); + } + @Test default void enqueueWorkChunkForProcessing_enqueuesOnlyREADYChunks() throws InterruptedException { // setup diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java index 48bf7264f35..6fb3b0e2787 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java @@ -26,15 +26,12 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertNull; import ca.uhn.fhir.batch2.model.JobDefinition; -import ca.uhn.fhir.batch2.model.JobInstance; -import ca.uhn.fhir.batch2.model.WorkChunk; import ca.uhn.fhir.batch2.model.WorkChunkCompletionEvent; import ca.uhn.fhir.batch2.model.WorkChunkErrorEvent; import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; import ca.uhn.hapi.fhir.batch2.test.support.JobMaintenanceStateInformation; import ca.uhn.test.concurrency.PointcutLatch; import com.google.common.collect.ImmutableList; -import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; @@ -55,7 +52,7 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC JobInstance instance = createInstance(); String instanceId = getTestManager().getSvc().storeNewInstance(instance); - String id = getTestManager().storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null, false); + String id = getTestManager().storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 0, null, false); getTestManager().runInTransaction(() -> { WorkChunk chunk = getTestManager().freshFetchWorkChunk(id); @@ -74,7 +71,7 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC getTestManager().enableMaintenanceRunner(false); - String id = getTestManager().storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA, theGatedExecution); + String id = getTestManager().storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 0, CHUNK_DATA, theGatedExecution); assertNotNull(id); getTestManager().runInTransaction(() -> assertEquals(expectedStatus, getTestManager().freshFetchWorkChunk(id).getStatus())); diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/WorkChunkTestConstants.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/WorkChunkTestConstants.java index 8ab0157e52b..10df1376af9 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/WorkChunkTestConstants.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/WorkChunkTestConstants.java @@ -24,7 +24,9 @@ public interface WorkChunkTestConstants { // we use a separate id for gated jobs because these job definitions might not // be cleaned up after any given test run String GATED_JOB_DEFINITION_ID = "gated_job_def_id"; - public static final String TARGET_STEP_ID = "step-id"; + public static final String FIRST_STEP_ID = "step-id"; + public static final String SECOND_STEP_ID = "2nd-step-id"; + public static final String LAST_STEP_ID = "last-step-id"; public static final String DEF_CHUNK_ID = "definition-chunkId"; public static final int JOB_DEF_VER = 1; public static final int SEQUENCE_NUMBER = 1; diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java index 455d3d2380a..8570d4b9252 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java @@ -99,9 +99,9 @@ public class JobInstanceProcessor { JobDefinition jobDefinition = myJobDefinitionegistry.getJobDefinitionOrThrowException(theInstance); - enqueueReadyChunks(theInstance, jobDefinition, false); cleanupInstance(theInstance); triggerGatedExecutions(theInstance, jobDefinition); + enqueueReadyChunks(theInstance, jobDefinition); ourLog.debug("Finished job processing: {} - {}", myInstanceId, stopWatch); } @@ -192,19 +192,15 @@ public class JobInstanceProcessor { String instanceId = theInstance.getInstanceId(); String currentStepId = jobWorkCursor.getCurrentStepId(); - boolean canAdvance = canAdvanceGatedJob(theJobDefinition, theInstance, jobWorkCursor); + boolean canAdvance = canAdvanceGatedJob(theInstance); if (canAdvance) { if (jobWorkCursor.isReductionStep()) { // current step is the reduction step (all reduction steps are final) JobWorkCursor nextJobWorkCursor = JobWorkCursor.fromJobDefinitionAndRequestedStepId( jobWorkCursor.getJobDefinition(), jobWorkCursor.getCurrentStepId()); myReductionStepExecutorService.triggerReductionStep(instanceId, nextJobWorkCursor); - } else if (jobWorkCursor.isFinalStep()) { - // current step is the final step in a non-reduction gated job - processChunksForNextGatedSteps( - theInstance, theJobDefinition, jobWorkCursor, jobWorkCursor.getCurrentStepId()); - } else { - // all other gated job steps + } else if (!jobWorkCursor.isFinalStep()) { + // all other gated job steps except for final steps String nextStepId = jobWorkCursor.nextStep.getStepId(); ourLog.info( "All processing is complete for gated execution of instance {} step {}. Proceeding to step {}", @@ -213,7 +209,7 @@ public class JobInstanceProcessor { nextStepId); // otherwise, continue processing as expected - processChunksForNextGatedSteps(theInstance, theJobDefinition, jobWorkCursor, nextStepId); + processChunksForNextGatedSteps(theInstance, nextStepId); } } else { String stepId = jobWorkCursor.nextStep != null @@ -227,8 +223,7 @@ public class JobInstanceProcessor { } } - private boolean canAdvanceGatedJob( - JobDefinition theJobDefinition, JobInstance theInstance, JobWorkCursor theWorkCursor) { + private boolean canAdvanceGatedJob(JobInstance theInstance) { // make sure our instance still exists if (myJobPersistence.fetchInstance(theInstance.getInstanceId()).isEmpty()) { // no more job @@ -245,49 +240,9 @@ public class JobInstanceProcessor { return true; } - if (workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.GATE_WAITING)) && theWorkCursor.isFirstStep()) { - // We are in the first step and all chunks are in GATE_WAITING - // this means that the job has just started, no workchunks have been queued yet -> proceed. - return true; - } - - if (workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.COMPLETED))) { - // all workchunks for the current step are in COMPLETED -> proceed. - return true; - } - - // all workchunks for gated jobs should be turned to QUEUED immediately after they are set to READY - // but in case we die in between, this conditional ought to catch the READY chunks. - if (workChunkStatuses.contains(WorkChunkStatusEnum.READY)) { - if (theWorkCursor.isFirstStep()) { - // first step - all ready means we're ready to proceed to enqueue - return true; - } else { - // it's a future step; - // make sure previous step's workchunks are completed - JobDefinitionStep previousStep = - theJobDefinition.getSteps().get(0); - for (JobDefinitionStep step : theJobDefinition.getSteps()) { - if (step.getStepId().equalsIgnoreCase(currentGatedStepId)) { - break; - } - previousStep = step; - } - Set previousStepWorkChunkStates = - myJobPersistence.getDistinctWorkChunkStatesForJobAndStep( - theInstance.getInstanceId(), previousStep.getStepId()); - - // completed means "all in COMPLETE state" or no previous chunks (they're cleaned up or never existed) - if (previousStepWorkChunkStates.isEmpty() - || previousStepWorkChunkStates.equals(Set.of(WorkChunkStatusEnum.COMPLETED))) { - return true; - } - } - } - - // anything else - return false; - } + // all workchunks for the current step are in COMPLETED -> proceed. + return workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.COMPLETED)); + } protected PagingIterator getReadyChunks() { return new PagingIterator<>(WORK_CHUNK_METADATA_BATCH_SIZE, (index, batchsize, consumer) -> { @@ -311,27 +266,12 @@ public class JobInstanceProcessor { * completing prematurely. */ private void enqueueReadyChunks( - JobInstance theJobInstance, JobDefinition theJobDefinition, boolean theIsGatedExecutionBool) { + JobInstance theJobInstance, JobDefinition theJobDefinition) { Iterator iter = getReadyChunks(); AtomicInteger counter = new AtomicInteger(); - ConcurrentHashMap> stepToWorkCursor = new ConcurrentHashMap<>(); while (iter.hasNext()) { WorkChunkMetadata metadata = iter.next(); - JobWorkCursor jobWorkCursor = stepToWorkCursor.computeIfAbsent(metadata.getTargetStepId(), (e) -> { - return JobWorkCursor.fromJobDefinitionAndRequestedStepId(theJobDefinition, metadata.getTargetStepId()); - }); - counter.getAndIncrement(); - if (!theIsGatedExecutionBool && (theJobDefinition.isGatedExecution() || jobWorkCursor.isReductionStep())) { - /* - * Gated executions are queued later when all work chunks are ready. - * - * Reduction steps are not submitted to the queue at all, but processed inline. - * Currently all reduction steps are also gated, but this might not always - * be true. - */ - return; - } /* * For each chunk id @@ -343,7 +283,7 @@ public class JobInstanceProcessor { updateChunkAndSendToQueue(metadata); } ourLog.debug( - "Encountered {} READY work chunks for job {}", counter.get(), theJobDefinition.getJobDefinitionId()); + "Encountered {} READY work chunks for job {} of type {}", counter.get(), theJobInstance.getInstanceId(), theJobDefinition.getJobDefinitionId()); } /** @@ -388,8 +328,6 @@ public class JobInstanceProcessor { private void processChunksForNextGatedSteps( JobInstance theInstance, - JobDefinition theJobDefinition, - JobWorkCursor theWorkCursor, String nextStepId) { String instanceId = theInstance.getInstanceId(); List readyChunksForNextStep = @@ -404,38 +342,7 @@ public class JobInstanceProcessor { readyChunksForNextStep.size()); } - boolean isEnqueue; - String currentStepId = theWorkCursor.getCurrentStepId(); - Set workChunkStatusesForCurrentStep = - myJobPersistence.getDistinctWorkChunkStatesForJobAndStep(theInstance.getInstanceId(), currentStepId); - if (workChunkStatusesForCurrentStep.equals(Set.of(WorkChunkStatusEnum.GATE_WAITING)) - && theWorkCursor.isFirstStep()) { - // this means that the job has just started, no workchunks have been queued yet - // turn the first chunk to READY, do NOT advance the step. - isEnqueue = myJobPersistence.updateAllChunksForStepWithStatus( - instanceId, currentStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.GATE_WAITING) - != 0; - } else if (workChunkStatusesForCurrentStep.equals(Set.of(WorkChunkStatusEnum.COMPLETED))) { - // update the job step so the workers will process them. - // if it's the last (gated) step, there will be no change - but we should - // queue up the chunks anyway - isEnqueue = theWorkCursor.isFinalStep() - || myJobPersistence.advanceJobStepAndUpdateChunkStatus(instanceId, nextStepId); - } else { - // this means that the current step's chunks contains only READY and QUEUED chunks, possibly leftover from - // other maintenance job who died in the middle - // should enqueue the rest of the ready chunks - isEnqueue = true; - } - - if (!isEnqueue) { - // we collided with another maintenance job. - ourLog.warn("Skipping gate advance to {} for instance {} - already advanced.", nextStepId, instanceId); - return; - } - - // because we now have all gated job chunks in READY state, - // we can enqueue them - enqueueReadyChunks(theInstance, theJobDefinition, true); + // update the job step so the workers will process them. + myJobPersistence.advanceJobStepAndUpdateChunkStatus(instanceId, nextStepId); } } From 2518531417959510cad88fba82cc408b43f38ffd Mon Sep 17 00:00:00 2001 From: tyner Date: Wed, 10 Apr 2024 08:31:27 -0400 Subject: [PATCH 04/13] - hid the over-powered transition function behind a proper state action --- .../jpa/batch2/JpaJobPersistenceImpl.java | 35 +++++------------- .../dao/data/IBatch2WorkChunkRepository.java | 16 +++++--- .../jpa/batch2/JpaJobPersistenceImplTest.java | 22 ++--------- .../jpa/batch2/JpaJobPersistenceImplTest.java | 37 ++++++++++++++----- .../uhn/fhir/batch2/api/IJobPersistence.java | 10 ++--- 5 files changed, 54 insertions(+), 66 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java index 89f56e671a4..8ef47b4ba25 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java @@ -598,22 +598,12 @@ public class JpaJobPersistenceImpl implements IJobPersistence { theJobInstanceId, theNextStepId); // when we reach here, the current step id is equal to theNextStepId - myWorkChunkRepository.updateAllChunksForStepWithStatus( - theJobInstanceId, theNextStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.GATE_WAITING); - - // In the old code, gated jobs' workchunks are created in status QUEUED but not actually queued for the - // workers. - // In order to keep them compatible, turn QUEUED chunks into READY, too. - // TODO: remove this when we are certain that no one is still running the old code. - int numChanged = myWorkChunkRepository.updateAllChunksForStepWithStatus( - theJobInstanceId, theNextStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.QUEUED); - if (numChanged > 0) { - ourLog.debug( - "Updated {} chunks of gated instance {} for step {} from fake QUEUED to READY.", - numChanged, - theJobInstanceId, - theNextStepId); - } + int numChanged = myWorkChunkRepository.updateAllChunksForStepFromGateWaitingToReady(theJobInstanceId, theNextStepId); + ourLog.debug( + "Updated {} chunks of gated instance {} for step {} from fake QUEUED to READY.", + numChanged, + theJobInstanceId, + theNextStepId); } return changed; @@ -621,18 +611,11 @@ public class JpaJobPersistenceImpl implements IJobPersistence { @Override @Transactional(propagation = Propagation.REQUIRES_NEW) - public int updateAllChunksForStepWithStatus( - String theJobInstanceId, - String theStepId, - WorkChunkStatusEnum theNewStatus, - WorkChunkStatusEnum theOldStatus) { + public int updateAllChunksForStepFromGateWaitingToReady(String theJobInstanceId, String theStepId) { ourLog.debug( - "Updating chunk status from {} to {} for gated instance {} in step {}.", - theOldStatus, - theNewStatus, + "Updating chunk status from GATE_WAITING to READY for gated instance {} in step {}.", theJobInstanceId, theStepId); - return myWorkChunkRepository.updateAllChunksForStepWithStatus( - theJobInstanceId, theStepId, theNewStatus, theOldStatus); + return myWorkChunkRepository.updateAllChunksForStepFromGateWaitingToReady(theJobInstanceId, theStepId); } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java index 93cd023c839..cbe530b1b18 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java @@ -109,14 +109,20 @@ public interface IBatch2WorkChunkRepository @Param("newStatus") WorkChunkStatusEnum theNewStatus, @Param("oldStatus") WorkChunkStatusEnum theOldStatus); + // In the old code, gated jobs' workchunks are created in status QUEUED but not actually queued for the + // workers. + // In order to keep them compatible, turn QUEUED chunks into READY, too. + // TODO: remove QUEUED from the in clause when we are certain that no one is still running the old code. @Modifying @Query( - "UPDATE Batch2WorkChunkEntity e SET e.myStatus = :newStatus WHERE e.myInstanceId = :instanceId AND e.myTargetStepId = :stepId AND e.myStatus = :oldStatus") - int updateAllChunksForStepWithStatus( + "UPDATE Batch2WorkChunkEntity e SET e.myStatus = ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.READY WHERE " + + "e.myInstanceId = :instanceId AND e.myTargetStepId = :stepId AND e.myStatus in (" + + "ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.GATE_WAITING," + + "ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.QUEUED" + + ")") + int updateAllChunksForStepFromGateWaitingToReady( @Param("instanceId") String theInstanceId, - @Param("stepId") String theStepId, - @Param("newStatus") WorkChunkStatusEnum theNewStatus, - @Param("oldStatus") WorkChunkStatusEnum theOldStatus); + @Param("stepId") String theStepId); @Modifying @Query("DELETE FROM Batch2WorkChunkEntity e WHERE e.myInstanceId = :instanceId") diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index 1b1cb585d51..9d3542eff91 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -32,6 +32,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) @@ -174,34 +175,17 @@ class JpaJobPersistenceImplTest { assertEquals(instance.getInstanceId(), retInstance.get().getInstanceId()); } - @Test - void advanceJobStepAndUpdateChunkStatus_validRequest_callsPersistenceUpdateAndReturnsChanged() { - // setup - String instanceId = "jobId"; - String nextStepId = "nextStep"; - - // execute - mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, nextStepId); - - // verify - verify(mySvc).updateInstance(instanceId, any()); - verify(myWorkChunkRepository).updateAllChunksForStepWithStatus(instanceId, nextStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.GATE_WAITING); - verify(myWorkChunkRepository).updateAllChunksForStepWithStatus(instanceId, nextStepId, WorkChunkStatusEnum.READY, WorkChunkStatusEnum.QUEUED); - } - @Test void updateAllChunksForStepWithStatus_validRequest_callsPersistenceUpdateAndReturnsChanged() { // setup String instanceId = "jobId"; String nextStepId = "nextStep"; - WorkChunkStatusEnum expectedNewStatus = WorkChunkStatusEnum.READY; - WorkChunkStatusEnum expectedOldStatus = WorkChunkStatusEnum.GATE_WAITING; // execute - mySvc.updateAllChunksForStepWithStatus(instanceId, nextStepId, expectedNewStatus, expectedOldStatus); + mySvc.updateAllChunksForStepFromGateWaitingToReady(instanceId, nextStepId); // verify - verify(myWorkChunkRepository).updateAllChunksForStepWithStatus(instanceId, nextStepId, expectedNewStatus, expectedOldStatus); + verify(myWorkChunkRepository).updateAllChunksForStepFromGateWaitingToReady(instanceId, nextStepId); } private JobInstance createJobInstanceFromEntity(Batch2JobInstanceEntity theEntity) { diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index d872def6b83..3dc896c1be1 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -410,11 +410,30 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { }); } + @Test + public void advanceJobStepAndUpdateChunkStatus_whenAlreadyInTargetStep_DoesNotUpdateStepOrChunks() { + // setup + JobInstance instance = createInstance(true, true); + String instanceId = mySvc.storeNewInstance(instance); + String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); + String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); + + runInTransaction(() -> assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); + + // execute + runInTransaction(() -> mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, TARGET_STEP_ID)); + + // verify + runInTransaction(() -> { + assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); + assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); + assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId()); + }); + } + @Test public void updateAllChunksForStepWithStatus_forGatedJob_updatesChunkStatus() { // setup - WorkChunkStatusEnum expectedNoChangeStatus = WorkChunkStatusEnum.GATE_WAITING; - WorkChunkStatusEnum expectedChangedStatus = WorkChunkStatusEnum.COMPLETED; JobInstance instance = createInstance(true, true); String instanceId = mySvc.storeNewInstance(instance); String chunkIdFirstStep = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null, true); @@ -422,19 +441,19 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); runInTransaction(() -> { - assertEquals(expectedNoChangeStatus, findChunkByIdOrThrow(chunkIdFirstStep).getStatus()); - assertEquals(expectedNoChangeStatus, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); - assertEquals(expectedNoChangeStatus, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); + assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdFirstStep).getStatus()); + assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); + assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); }); // execute - runInTransaction(() -> mySvc.updateAllChunksForStepWithStatus(instanceId, LAST_STEP_ID, expectedChangedStatus, WorkChunkStatusEnum.GATE_WAITING)); + runInTransaction(() -> mySvc.updateAllChunksForStepFromGateWaitingToReady(instanceId, LAST_STEP_ID)); // verify runInTransaction(() -> { - assertEquals(expectedNoChangeStatus, findChunkByIdOrThrow(chunkIdFirstStep).getStatus()); - assertEquals(expectedChangedStatus, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); - assertEquals(expectedChangedStatus, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); + assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdFirstStep).getStatus()); + assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); + assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); }); } diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java index 828aee61225..e4611a176fa 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java @@ -291,17 +291,13 @@ public interface IJobPersistence extends IWorkChunkPersistence { boolean advanceJobStepAndUpdateChunkStatus(String theJobInstanceId, String theNextStepId); /** - * Update all chunks of the given step id for the given job from + * Update all chunks of the given step id for the given job from GATE_WAITING to READY * @param theJobInstanceId the id of the job instance to be updated * @param theStepId the id of the step which the chunks belong to - * @param theNewStatus the new status - * @param theOldStatus the old status * @return */ @Transactional(propagation = Propagation.REQUIRES_NEW) - int updateAllChunksForStepWithStatus( + int updateAllChunksForStepFromGateWaitingToReady( String theJobInstanceId, - String theStepId, - WorkChunkStatusEnum theNewStatus, - WorkChunkStatusEnum theOldStatus); + String theStepId); } From d06337d10312d68a8badac094bec46dda8718d49 Mon Sep 17 00:00:00 2001 From: tyner Date: Wed, 10 Apr 2024 08:32:28 -0400 Subject: [PATCH 05/13] spotless --- .../jpa/batch2/JpaJobPersistenceImpl.java | 3 ++- .../dao/data/IBatch2WorkChunkRepository.java | 14 ++++++------- .../uhn/fhir/batch2/api/IJobPersistence.java | 4 +--- .../maintenance/JobInstanceProcessor.java | 20 +++++++++---------- 4 files changed, 18 insertions(+), 23 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java index 8ef47b4ba25..e45d896dc4b 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java @@ -598,7 +598,8 @@ public class JpaJobPersistenceImpl implements IJobPersistence { theJobInstanceId, theNextStepId); // when we reach here, the current step id is equal to theNextStepId - int numChanged = myWorkChunkRepository.updateAllChunksForStepFromGateWaitingToReady(theJobInstanceId, theNextStepId); + int numChanged = + myWorkChunkRepository.updateAllChunksForStepFromGateWaitingToReady(theJobInstanceId, theNextStepId); ourLog.debug( "Updated {} chunks of gated instance {} for step {} from fake QUEUED to READY.", numChanged, diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java index cbe530b1b18..03a6524f4fc 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java @@ -114,15 +114,13 @@ public interface IBatch2WorkChunkRepository // In order to keep them compatible, turn QUEUED chunks into READY, too. // TODO: remove QUEUED from the in clause when we are certain that no one is still running the old code. @Modifying - @Query( - "UPDATE Batch2WorkChunkEntity e SET e.myStatus = ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.READY WHERE " - + "e.myInstanceId = :instanceId AND e.myTargetStepId = :stepId AND e.myStatus in (" - + "ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.GATE_WAITING," - + "ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.QUEUED" - + ")") + @Query("UPDATE Batch2WorkChunkEntity e SET e.myStatus = ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.READY WHERE " + + "e.myInstanceId = :instanceId AND e.myTargetStepId = :stepId AND e.myStatus in (" + + "ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.GATE_WAITING," + + "ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.QUEUED" + + ")") int updateAllChunksForStepFromGateWaitingToReady( - @Param("instanceId") String theInstanceId, - @Param("stepId") String theStepId); + @Param("instanceId") String theInstanceId, @Param("stepId") String theStepId); @Modifying @Query("DELETE FROM Batch2WorkChunkEntity e WHERE e.myInstanceId = :instanceId") diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java index e4611a176fa..c11aec286ce 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java @@ -297,7 +297,5 @@ public interface IJobPersistence extends IWorkChunkPersistence { * @return */ @Transactional(propagation = Propagation.REQUIRES_NEW) - int updateAllChunksForStepFromGateWaitingToReady( - String theJobInstanceId, - String theStepId); + int updateAllChunksForStepFromGateWaitingToReady(String theJobInstanceId, String theStepId); } diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java index 8570d4b9252..63a6dae1f36 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java @@ -24,7 +24,6 @@ import ca.uhn.fhir.batch2.api.IReductionStepExecutorService; import ca.uhn.fhir.batch2.channel.BatchJobSender; import ca.uhn.fhir.batch2.coordinator.JobDefinitionRegistry; import ca.uhn.fhir.batch2.model.JobDefinition; -import ca.uhn.fhir.batch2.model.JobDefinitionStep; import ca.uhn.fhir.batch2.model.JobInstance; import ca.uhn.fhir.batch2.model.JobWorkCursor; import ca.uhn.fhir.batch2.model.JobWorkNotification; @@ -45,7 +44,6 @@ import org.springframework.data.domain.Pageable; import java.util.Iterator; import java.util.List; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicInteger; public class JobInstanceProcessor { @@ -240,9 +238,9 @@ public class JobInstanceProcessor { return true; } - // all workchunks for the current step are in COMPLETED -> proceed. - return workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.COMPLETED)); - } + // all workchunks for the current step are in COMPLETED -> proceed. + return workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.COMPLETED)); + } protected PagingIterator getReadyChunks() { return new PagingIterator<>(WORK_CHUNK_METADATA_BATCH_SIZE, (index, batchsize, consumer) -> { @@ -265,8 +263,7 @@ public class JobInstanceProcessor { * we'd need a new GATE_WAITING state to move chunks to prevent jobs from * completing prematurely. */ - private void enqueueReadyChunks( - JobInstance theJobInstance, JobDefinition theJobDefinition) { + private void enqueueReadyChunks(JobInstance theJobInstance, JobDefinition theJobDefinition) { Iterator iter = getReadyChunks(); AtomicInteger counter = new AtomicInteger(); @@ -283,7 +280,10 @@ public class JobInstanceProcessor { updateChunkAndSendToQueue(metadata); } ourLog.debug( - "Encountered {} READY work chunks for job {} of type {}", counter.get(), theJobInstance.getInstanceId(), theJobDefinition.getJobDefinitionId()); + "Encountered {} READY work chunks for job {} of type {}", + counter.get(), + theJobInstance.getInstanceId(), + theJobDefinition.getJobDefinitionId()); } /** @@ -326,9 +326,7 @@ public class JobInstanceProcessor { myBatchJobSender.sendWorkChannelMessage(workNotification); } - private void processChunksForNextGatedSteps( - JobInstance theInstance, - String nextStepId) { + private void processChunksForNextGatedSteps(JobInstance theInstance, String nextStepId) { String instanceId = theInstance.getInstanceId(); List readyChunksForNextStep = myProgressAccumulator.getChunkIdsWithStatus(instanceId, nextStepId, WorkChunkStatusEnum.READY); From fd4fdd754122afa9ce43c4b58cfc0a33aad70537 Mon Sep 17 00:00:00 2001 From: tyner Date: Wed, 10 Apr 2024 09:17:44 -0400 Subject: [PATCH 06/13] resolved review comments --- .../fhir/docs/server_jpa_batch/batch2_states.md | 8 ++++---- .../fhir/docs/server_jpa_batch/introduction.md | 6 +++--- .../jpa/dao/data/IBatch2WorkChunkRepository.java | 4 ++-- .../jpa/batch2/JpaJobPersistenceImplTest.java | 3 ++- .../jpa/batch2/JpaJobPersistenceImplTest.java | 15 ++++++++++++--- .../AbstractIJobPersistenceSpecificationTest.java | 2 +- .../ca/uhn/fhir/batch2/api/IJobPersistence.java | 4 ++-- 7 files changed, 26 insertions(+), 16 deletions(-) diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md index 1db1e9f1d18..3524cb35370 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md @@ -56,10 +56,10 @@ stateDiagram-v2 state FAILED state COMPLETED direction LR - [*] --> READY : on create - normal - [*] --> GATE_WAITING : on create - gated - GATE_WAITING --> READY : on gate release - gated (new) - QUEUED --> READY : on gate release - gated (for compatibility with old "fake QUEUED" state) + [*] --> READY : on create - normal or gated first chunk + [*] --> GATE_WAITING : on create - gated non-first chunk + GATE_WAITING --> READY : on gate release - gated + QUEUED --> READY : on gate release - gated (for compatibility with legacy QUEUED state up to 7.1.8-SNAPSHOT) READY --> QUEUED : placed on kafka (maint.) %% worker processing states diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md index e05cb1c54ec..dc59e9a2d09 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md @@ -20,17 +20,17 @@ After a job has been defined, *instances* of that job can be submitted for batch The Batch Job Coordinator will then store two records in the database: - Job Instance with status `QUEUED`: that is the parent record for all data concerning this job -- Batch Work Chunk with status `READY`/`GATE_WAITING`: this describes the first "chunk" of work required for this job. The first Batch Work Chunk contains no data. The initial status of the work chunk will be `READY` or `GATE_WAITING` for non-gated and gated batch jobs, respectively. Please refer to [Gated Execution](#gated-execution) for more explanation on gated batch jobs. +- Batch Work Chunk with status `READY`: this describes the first "chunk" of work required for this job. The first Batch Work Chunk contains no data. ### The Maintenance Job A Scheduled Job runs periodically (once a minute). For each Job Instance in the database, it: -1. Moves all `READY` work chunks into the `QUEUED` state and publishes a message to the Batch Notification Message Channel to inform worker threads that a work chunk is now ready for processing. For gated batch jobs, the maintenance also moves all `GATE_WAITING` work chunks into `READY` when the current batch step is ready to advance. \* 1. Calculates job progress (% of work chunks in `COMPLETE` status). If the job is finished, purges any left over work chunks still in the database. 1. Cleans up any complete, failed, or cancelled jobs that need to be removed. -1. Moves any gated jobs onto their next step when complete. +1. When the current step is complete, moves any gated jobs onto their next step and update all chunks in `GATE_WAITING` to `READY`. 1. If the final step of a gated job is a reduction step, a reduction step execution will be triggered. +1. Moves all `READY` work chunks into the `QUEUED` state and publishes a message to the Batch Notification Message Channel to inform worker threads that a work chunk is now ready for processing. \* \* An exception is for the final reduction step, where work chunks are not published to the Batch Notification Message Channel, but instead processed inline. diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java index 03a6524f4fc..3b662a7eede 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java @@ -109,10 +109,10 @@ public interface IBatch2WorkChunkRepository @Param("newStatus") WorkChunkStatusEnum theNewStatus, @Param("oldStatus") WorkChunkStatusEnum theOldStatus); - // In the old code, gated jobs' workchunks are created in status QUEUED but not actually queued for the + // Up to 7.1.8-SNAPSHOT, gated jobs' work chunks are created in status QUEUED but not actually queued for the // workers. // In order to keep them compatible, turn QUEUED chunks into READY, too. - // TODO: remove QUEUED from the in clause when we are certain that no one is still running the old code. + // TODO: remove QUEUED from the IN clause when we are certain that no one is still running the old code. @Modifying @Query("UPDATE Batch2WorkChunkEntity e SET e.myStatus = ca.uhn.fhir.batch2.model.WorkChunkStatusEnum.READY WHERE " + "e.myInstanceId = :instanceId AND e.myTargetStepId = :stepId AND e.myStatus in (" diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index 9d3542eff91..9b36c866961 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -182,9 +182,10 @@ class JpaJobPersistenceImplTest { String nextStepId = "nextStep"; // execute - mySvc.updateAllChunksForStepFromGateWaitingToReady(instanceId, nextStepId); + int changed = mySvc.updateAllChunksForStepFromGateWaitingToReady(instanceId, nextStepId); // verify + assertEquals(0, changed); verify(myWorkChunkRepository).updateAllChunksForStepFromGateWaitingToReady(instanceId, nextStepId); } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index 3dc896c1be1..96743600a3b 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -400,7 +400,10 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { runInTransaction(() -> assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); // execute - runInTransaction(() -> mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, LAST_STEP_ID)); + runInTransaction(() -> { + boolean changed = mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, LAST_STEP_ID); + assertTrue(changed); + }); // verify runInTransaction(() -> { @@ -421,7 +424,10 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { runInTransaction(() -> assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); // execute - runInTransaction(() -> mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, TARGET_STEP_ID)); + runInTransaction(() -> { + boolean changed = mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, TARGET_STEP_ID); + assertFalse(changed); + }); // verify runInTransaction(() -> { @@ -447,7 +453,10 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { }); // execute - runInTransaction(() -> mySvc.updateAllChunksForStepFromGateWaitingToReady(instanceId, LAST_STEP_ID)); + runInTransaction(() -> { + int numChanged = mySvc.updateAllChunksForStepFromGateWaitingToReady(instanceId, LAST_STEP_ID); + assertEquals(2, numChanged); + }); // verify runInTransaction(() -> { diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java index 0d2a00bc622..98df1de615d 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java @@ -166,7 +166,7 @@ public abstract class AbstractIJobPersistenceSpecificationTest implements IJobMa instance.setJobDefinitionVersion(JOB_DEF_VER); instance.setParameters(CHUNK_DATA); instance.setReport("TEST"); - if (jobDefinition.isGatedExecution()) { + if (jobDefinition.isGatedExecution()) { instance.setCurrentGatedStepId(jobDefinition.getFirstStepId()); } return instance; diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java index c11aec286ce..f47725fa411 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java @@ -285,7 +285,7 @@ public interface IJobPersistence extends IWorkChunkPersistence { * Atomically advance the given job to the given step and change the status of all chunks in the next step to READY * @param theJobInstanceId the id of the job instance to be updated * @param theNextStepId the id of the next job step - * @return + * @return whether any changes were made */ @Transactional(propagation = Propagation.REQUIRES_NEW) boolean advanceJobStepAndUpdateChunkStatus(String theJobInstanceId, String theNextStepId); @@ -294,7 +294,7 @@ public interface IJobPersistence extends IWorkChunkPersistence { * Update all chunks of the given step id for the given job from GATE_WAITING to READY * @param theJobInstanceId the id of the job instance to be updated * @param theStepId the id of the step which the chunks belong to - * @return + * @return the number of chunks updated */ @Transactional(propagation = Propagation.REQUIRES_NEW) int updateAllChunksForStepFromGateWaitingToReady(String theJobInstanceId, String theStepId); From decdeafdc11af55dc192f74d8d06111e7970e9f5 Mon Sep 17 00:00:00 2001 From: tyner Date: Wed, 10 Apr 2024 16:59:52 -0400 Subject: [PATCH 07/13] resolved review comments --- .../docs/server_jpa_batch/batch2_states.md | 4 +- .../docs/server_jpa_batch/introduction.md | 2 +- .../jpa/batch2/JpaJobPersistenceImpl.java | 3 +- .../dao/data/IBatch2WorkChunkRepository.java | 2 +- .../batch2/test/IJobMaintenanceActions.java | 5 +- .../JobMaintenanceStateInformation.java | 2 +- .../uhn/fhir/batch2/api/IJobPersistence.java | 3 +- .../maintenance/JobInstanceProcessor.java | 48 ++++++++++++------- 8 files changed, 42 insertions(+), 27 deletions(-) diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md index 3524cb35370..3e848564ea0 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md @@ -57,9 +57,9 @@ stateDiagram-v2 state COMPLETED direction LR [*] --> READY : on create - normal or gated first chunk - [*] --> GATE_WAITING : on create - gated non-first chunk + [*] --> GATE_WAITING : on create - gated jobs for all but the first chunk GATE_WAITING --> READY : on gate release - gated - QUEUED --> READY : on gate release - gated (for compatibility with legacy QUEUED state up to 7.1.8-SNAPSHOT) + QUEUED --> READY : on gate release - gated (for compatibility with legacy QUEUED state up to Hapi-fhir version 7.1) READY --> QUEUED : placed on kafka (maint.) %% worker processing states diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md index dc59e9a2d09..62e8a4f5871 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md @@ -28,7 +28,7 @@ A Scheduled Job runs periodically (once a minute). For each Job Instance in the 1. Calculates job progress (% of work chunks in `COMPLETE` status). If the job is finished, purges any left over work chunks still in the database. 1. Cleans up any complete, failed, or cancelled jobs that need to be removed. -1. When the current step is complete, moves any gated jobs onto their next step and update all chunks in `GATE_WAITING` to `READY`. +1. When the current step is complete, moves any gated jobs onto their next step and updates all chunks in `GATE_WAITING` to `READY`. 1. If the final step of a gated job is a reduction step, a reduction step execution will be triggered. 1. Moves all `READY` work chunks into the `QUEUED` state and publishes a message to the Batch Notification Message Channel to inform worker threads that a work chunk is now ready for processing. \* diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java index e45d896dc4b..bec1bc31349 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java @@ -128,7 +128,8 @@ public class JpaJobPersistenceImpl implements IJobPersistence { entity.setSerializedData(theBatchWorkChunk.serializedData); entity.setCreateTime(new Date()); entity.setStartTime(new Date()); - // set to GATE_WAITING if job is gated, to READY if not + // set gated job chunks to GATE_WAITING; they will be transitioned to READY during maintenance pass when all + // chunks in the previous step are COMPLETED entity.setStatus( theBatchWorkChunk.isGatedExecution ? WorkChunkStatusEnum.GATE_WAITING : WorkChunkStatusEnum.READY); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java index 3b662a7eede..44a6f4eb1c4 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/data/IBatch2WorkChunkRepository.java @@ -109,7 +109,7 @@ public interface IBatch2WorkChunkRepository @Param("newStatus") WorkChunkStatusEnum theNewStatus, @Param("oldStatus") WorkChunkStatusEnum theOldStatus); - // Up to 7.1.8-SNAPSHOT, gated jobs' work chunks are created in status QUEUED but not actually queued for the + // Up to 7.1, gated jobs' work chunks are created in status QUEUED but not actually queued for the // workers. // In order to keep them compatible, turn QUEUED chunks into READY, too. // TODO: remove QUEUED from the IN clause when we are certain that no one is still running the old code. diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java index 843ddecb7c8..5426fc54948 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java @@ -4,7 +4,6 @@ import ca.uhn.fhir.batch2.model.JobDefinition; import ca.uhn.hapi.fhir.batch2.test.support.JobMaintenanceStateInformation; import ca.uhn.test.concurrency.PointcutLatch; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -21,7 +20,7 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC } @Test - default void test_gatedJob_stepReady_advances() throws InterruptedException { + default void test_gatedJob_stepReady_stepAdvances() throws InterruptedException { // setup String initialState = """ # chunks ready - move to queued @@ -108,7 +107,7 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC 3|GATE_WAITING """ }) - default void testGatedStep2NotReady_notAdvance(String theChunkState) throws InterruptedException { + default void testGatedStep2NotReady_stepNotAdvance(String theChunkState) throws InterruptedException { // setup int expectedLatchCount = getLatchCountFromState(theChunkState); PointcutLatch sendingLatch = getTestManager().disableWorkChunkMessageHandler(); diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/support/JobMaintenanceStateInformation.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/support/JobMaintenanceStateInformation.java index 511f675c2e6..fa70acdf247 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/support/JobMaintenanceStateInformation.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/support/JobMaintenanceStateInformation.java @@ -145,7 +145,7 @@ public class JobMaintenanceStateInformation { if (jobDef.isGatedExecution()) { AtomicReference latestStepId = new AtomicReference<>(); int totalSteps = jobDef.getSteps().size(); - // ignore the last step + // ignore the last step since tests in gated jobs needs the current step to be the second-last step for (int i = totalSteps - 2; i >= 0; i--) { JobDefinitionStep step = jobDef.getSteps().get(i); if (stepIds.contains(step.getStepId())) { diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java index f47725fa411..83a9cfcc73d 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java @@ -282,7 +282,8 @@ public interface IJobPersistence extends IWorkChunkPersistence { WorkChunk createWorkChunk(WorkChunk theWorkChunk); /** - * Atomically advance the given job to the given step and change the status of all chunks in the next step to READY + * Atomically advance the given job to the given step and change the status of all QUEUED and GATE_WAITING chunks + * in the next step to READY * @param theJobInstanceId the id of the job instance to be updated * @param theNextStepId the id of the next job step * @return whether any changes were made diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java index 63a6dae1f36..0a7321a3629 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java @@ -44,7 +44,6 @@ import org.springframework.data.domain.Pageable; import java.util.Iterator; import java.util.List; import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; public class JobInstanceProcessor { private static final Logger ourLog = Logs.getBatchTroubleshootingLog(); @@ -99,6 +98,18 @@ public class JobInstanceProcessor { cleanupInstance(theInstance); triggerGatedExecutions(theInstance, jobDefinition); + + if (theInstance.hasGatedStep()) { + JobWorkCursor jobWorkCursor = JobWorkCursor.fromJobDefinitionAndRequestedStepId( + jobDefinition, theInstance.getCurrentGatedStepId()); + if (jobWorkCursor.isReductionStep()) { + // Reduction step work chunks should never be sent to the queue but to its specific service instead. + triggerReductionStep(theInstance, jobWorkCursor); + return; + } + } + + // enqueue the chunks as normal enqueueReadyChunks(theInstance, jobDefinition); ourLog.debug("Finished job processing: {} - {}", myInstanceId, stopWatch); @@ -187,18 +198,12 @@ public class JobInstanceProcessor { JobWorkCursor jobWorkCursor = JobWorkCursor.fromJobDefinitionAndRequestedStepId( theJobDefinition, theInstance.getCurrentGatedStepId()); - String instanceId = theInstance.getInstanceId(); String currentStepId = jobWorkCursor.getCurrentStepId(); boolean canAdvance = canAdvanceGatedJob(theInstance); if (canAdvance) { - if (jobWorkCursor.isReductionStep()) { - // current step is the reduction step (all reduction steps are final) - JobWorkCursor nextJobWorkCursor = JobWorkCursor.fromJobDefinitionAndRequestedStepId( - jobWorkCursor.getJobDefinition(), jobWorkCursor.getCurrentStepId()); - myReductionStepExecutorService.triggerReductionStep(instanceId, nextJobWorkCursor); - } else if (!jobWorkCursor.isFinalStep()) { - // all other gated job steps except for final steps + if (!jobWorkCursor.isFinalStep()) { + // all other gated job steps except for final steps - final steps does not need to be advanced String nextStepId = jobWorkCursor.nextStep.getStepId(); ourLog.info( "All processing is complete for gated execution of instance {} step {}. Proceeding to step {}", @@ -206,8 +211,12 @@ public class JobInstanceProcessor { currentStepId, nextStepId); - // otherwise, continue processing as expected processChunksForNextGatedSteps(theInstance, nextStepId); + } else { + ourLog.info( + "Ready to advance gated execution of instance {} but already at the final step {}. Not proceeding to advance steps.", + instanceId, + jobWorkCursor.getCurrentStepId()); } } else { String stepId = jobWorkCursor.nextStep != null @@ -253,20 +262,24 @@ public class JobInstanceProcessor { }); } + /** + * Trigger the reduction step for the given job instance. Reduction step chunks should never be queued. + */ + private void triggerReductionStep(JobInstance theInstance, JobWorkCursor jobWorkCursor) { + String instanceId = theInstance.getInstanceId(); + ourLog.debug("Triggering Reduction step {} of instance {}.", jobWorkCursor.getCurrentStepId(), instanceId); + myReductionStepExecutorService.triggerReductionStep(instanceId, jobWorkCursor); + } + /** * Chunks are initially created in READY state. * We will move READY chunks to QUEUE'd and send them to the queue/topic (kafka) * for processing. - * - * We could block chunks from being moved from QUEUE'd to READY here for gated steps - * but currently, progress is calculated by looking at completed chunks only; - * we'd need a new GATE_WAITING state to move chunks to prevent jobs from - * completing prematurely. */ private void enqueueReadyChunks(JobInstance theJobInstance, JobDefinition theJobDefinition) { Iterator iter = getReadyChunks(); - AtomicInteger counter = new AtomicInteger(); + int counter = 0; while (iter.hasNext()) { WorkChunkMetadata metadata = iter.next(); @@ -278,10 +291,11 @@ public class JobInstanceProcessor { * * commit */ updateChunkAndSendToQueue(metadata); + counter++; } ourLog.debug( "Encountered {} READY work chunks for job {} of type {}", - counter.get(), + counter, theJobInstance.getInstanceId(), theJobDefinition.getJobDefinitionId()); } From 292a4d3858d0101664fd3b24a83015b55940b536 Mon Sep 17 00:00:00 2001 From: tyner Date: Thu, 11 Apr 2024 15:03:24 -0400 Subject: [PATCH 08/13] resolved review comments --- .../jpa/batch2/JpaJobPersistenceImpl.java | 10 ---- .../jpa/batch2/JpaJobPersistenceImplTest.java | 14 ------ .../jpa/batch2/JpaJobPersistenceImplTest.java | 29 ----------- ...tractIJobPersistenceSpecificationTest.java | 4 -- .../hapi/fhir/batch2/test/ITestFixture.java | 6 ++- .../test/IWorkChunkStateTransitions.java | 48 ++++++++----------- .../uhn/fhir/batch2/api/IJobPersistence.java | 9 ---- 7 files changed, 24 insertions(+), 96 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java index bec1bc31349..33a7b9c6623 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java @@ -610,14 +610,4 @@ public class JpaJobPersistenceImpl implements IJobPersistence { return changed; } - - @Override - @Transactional(propagation = Propagation.REQUIRES_NEW) - public int updateAllChunksForStepFromGateWaitingToReady(String theJobInstanceId, String theStepId) { - ourLog.debug( - "Updating chunk status from GATE_WAITING to READY for gated instance {} in step {}.", - theJobInstanceId, - theStepId); - return myWorkChunkRepository.updateAllChunksForStepFromGateWaitingToReady(theJobInstanceId, theStepId); - } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index 9b36c866961..14b21559d52 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -175,20 +175,6 @@ class JpaJobPersistenceImplTest { assertEquals(instance.getInstanceId(), retInstance.get().getInstanceId()); } - @Test - void updateAllChunksForStepWithStatus_validRequest_callsPersistenceUpdateAndReturnsChanged() { - // setup - String instanceId = "jobId"; - String nextStepId = "nextStep"; - - // execute - int changed = mySvc.updateAllChunksForStepFromGateWaitingToReady(instanceId, nextStepId); - - // verify - assertEquals(0, changed); - verify(myWorkChunkRepository).updateAllChunksForStepFromGateWaitingToReady(instanceId, nextStepId); - } - private JobInstance createJobInstanceFromEntity(Batch2JobInstanceEntity theEntity) { return JobInstanceUtil.fromEntityToInstance(theEntity); } diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index 96743600a3b..6c9b2202221 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -437,35 +437,6 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { }); } - @Test - public void updateAllChunksForStepWithStatus_forGatedJob_updatesChunkStatus() { - // setup - JobInstance instance = createInstance(true, true); - String instanceId = mySvc.storeNewInstance(instance); - String chunkIdFirstStep = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, null, true); - String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); - String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); - - runInTransaction(() -> { - assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdFirstStep).getStatus()); - assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); - assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); - }); - - // execute - runInTransaction(() -> { - int numChanged = mySvc.updateAllChunksForStepFromGateWaitingToReady(instanceId, LAST_STEP_ID); - assertEquals(2, numChanged); - }); - - // verify - runInTransaction(() -> { - assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdFirstStep).getStatus()); - assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); - assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); - }); - } - @Test public void testFetchUnknownWork() { assertFalse(myWorkChunkRepository.findById("FOO").isPresent()); diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java index 98df1de615d..ddea3af0f91 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java @@ -245,10 +245,6 @@ public abstract class AbstractIJobPersistenceSpecificationTest implements IJobMa return storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, theInstanceId, 0, CHUNK_DATA, theGatedExecution); } - public String createChunkInStep(String theInstanceId, String theStepId, boolean theGatedExecution) { - return storeWorkChunk(JOB_DEFINITION_ID, theStepId, theInstanceId, 0, CHUNK_DATA, theGatedExecution); - } - public String createFirstChunk(JobDefinition theJobDefinition, String theJobInstanceId){ return storeFirstWorkChunk(theJobDefinition, theJobInstanceId); } diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java index 47a4230728a..cd155fb5a7f 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/ITestFixture.java @@ -64,8 +64,10 @@ public interface ITestFixture { String createChunk(String theJobInstanceId, boolean theGatedExecution); - String createChunkInStep(String theJobInstanceId, String theStepId, boolean theGatedExecution); - + /** + * Create chunk as the first chunk of a job. + * @return the id of the created chunk + */ String createFirstChunk(JobDefinition theJobDefinition, String theJobInstanceId); /** diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java index 5d9444e9664..aad98412c68 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java @@ -21,7 +21,6 @@ package ca.uhn.hapi.fhir.batch2.test; import ca.uhn.fhir.batch2.model.JobDefinition; import ca.uhn.fhir.batch2.model.WorkChunk; -import ca.uhn.fhir.batch2.model.WorkChunkCompletionEvent; import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; import ca.uhn.hapi.fhir.batch2.test.support.JobMaintenanceStateInformation; import ca.uhn.hapi.fhir.batch2.test.support.TestJobParameters; @@ -33,8 +32,6 @@ import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.ArrayList; - import static org.junit.jupiter.api.Assertions.assertEquals; public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkTestConstants { @@ -62,6 +59,7 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT String myChunkId = getTestManager().createFirstChunk(jobDef, jobInstanceId); WorkChunk fetchedWorkChunk = getTestManager().freshFetchWorkChunk(myChunkId); + // the first chunk of both gated and non-gated job should start in READY assertEquals(WorkChunkStatusEnum.READY, fetchedWorkChunk.getStatus(), "New chunks are " + WorkChunkStatusEnum.READY); } @@ -88,37 +86,31 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT } @Test - default void chunkReceived_forGatedJob_queuedToInProgress() throws InterruptedException { - PointcutLatch sendLatch = getTestManager().disableWorkChunkMessageHandler(); - sendLatch.setExpectedCount(2); + default void advanceJobStepAndUpdateChunkStatus_forGatedJob_updatesBothREADYAndQUEUEDChunks() { + // setup + getTestManager().disableWorkChunkMessageHandler(); + getTestManager().enableMaintenanceRunner(false); + + String state = """ + 1|COMPLETED + 2|COMPLETED + 3|GATE_WAITING,3|READY + 3|QUEUED,3|READY + """; JobDefinition jobDef = getTestManager().withJobDefinition(true); String jobInstanceId = getTestManager().createAndStoreJobInstance(jobDef); - String chunkInStep1 = getTestManager().createFirstChunk(jobDef, jobInstanceId); - String chunkInStep2 = getTestManager().createChunkInStep(jobInstanceId, SECOND_STEP_ID, true); - // dequeue and completes the first chunk - getTestManager().runMaintenancePass(); - // the worker has received the chunk, and marks it started. - WorkChunk chunk1 = getTestManager().getSvc().onWorkChunkDequeue(chunkInStep1).orElseThrow(IllegalArgumentException::new); - assertEquals(WorkChunkStatusEnum.IN_PROGRESS, chunk1.getStatus()); - WorkChunk fetchedWorkChunk = getTestManager().freshFetchWorkChunk(chunkInStep1); - assertEquals(WorkChunkStatusEnum.IN_PROGRESS, fetchedWorkChunk.getStatus()); + JobMaintenanceStateInformation info = new JobMaintenanceStateInformation(jobInstanceId, jobDef, state); + getTestManager().createChunksInStates(info); + assertEquals(SECOND_STEP_ID, getTestManager().freshFetchJobInstance(jobInstanceId).getCurrentGatedStepId()); - getTestManager().getSvc().onWorkChunkCompletion(new WorkChunkCompletionEvent(chunkInStep1, 50, 0)); - fetchedWorkChunk = getTestManager().freshFetchWorkChunk(chunkInStep1); - assertEquals(WorkChunkStatusEnum.COMPLETED, fetchedWorkChunk.getStatus()); + // execute + getTestManager().runInTransaction(() -> getTestManager().getSvc().advanceJobStepAndUpdateChunkStatus(jobInstanceId, LAST_STEP_ID)); - // dequeue the second chunk - getTestManager().runMaintenancePass(); - WorkChunk chunk2 = getTestManager().getSvc().onWorkChunkDequeue(chunkInStep2).orElseThrow(IllegalArgumentException::new); - assertEquals(WorkChunkStatusEnum.IN_PROGRESS, chunk2.getStatus()); - assertEquals(CHUNK_DATA, chunk2.getData()); - - WorkChunk fetchedWorkChunk2 = getTestManager().freshFetchWorkChunk(chunkInStep2); - assertEquals(WorkChunkStatusEnum.IN_PROGRESS, fetchedWorkChunk2.getStatus()); - - getTestManager().verifyWorkChunkMessageHandlerCalled(sendLatch, 2); + // verify + assertEquals(LAST_STEP_ID, getTestManager().freshFetchJobInstance(jobInstanceId).getCurrentGatedStepId()); + info.verifyFinalStates(getTestManager().getSvc()); } @Test diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java index 83a9cfcc73d..e8377c60061 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java @@ -290,13 +290,4 @@ public interface IJobPersistence extends IWorkChunkPersistence { */ @Transactional(propagation = Propagation.REQUIRES_NEW) boolean advanceJobStepAndUpdateChunkStatus(String theJobInstanceId, String theNextStepId); - - /** - * Update all chunks of the given step id for the given job from GATE_WAITING to READY - * @param theJobInstanceId the id of the job instance to be updated - * @param theStepId the id of the step which the chunks belong to - * @return the number of chunks updated - */ - @Transactional(propagation = Propagation.REQUIRES_NEW) - int updateAllChunksForStepFromGateWaitingToReady(String theJobInstanceId, String theStepId); } From 658da406a4ce23403dc4b185b619921d5a014520 Mon Sep 17 00:00:00 2001 From: tyner Date: Thu, 11 Apr 2024 15:17:26 -0400 Subject: [PATCH 09/13] resolved review comments --- .../AbstractIJobPersistenceSpecificationTest.java | 1 + .../batch2/test/IWorkChunkStateTransitions.java | 8 ++++++-- .../fhir/batch2/test/IWorkChunkStorageTests.java | 14 ++++++-------- 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java index ddea3af0f91..ab7affed055 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/AbstractIJobPersistenceSpecificationTest.java @@ -39,6 +39,7 @@ import ca.uhn.hapi.fhir.batch2.test.support.TestJobStep3InputType; import ca.uhn.test.concurrency.PointcutLatch; import jakarta.annotation.Nonnull; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.mockito.ArgumentCaptor; import org.mockito.Mockito; diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java index aad98412c68..96a8902da3e 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStateTransitions.java @@ -25,6 +25,7 @@ import ca.uhn.fhir.batch2.model.WorkChunkStatusEnum; import ca.uhn.hapi.fhir.batch2.test.support.JobMaintenanceStateInformation; import ca.uhn.hapi.fhir.batch2.test.support.TestJobParameters; import ca.uhn.test.concurrency.PointcutLatch; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.CsvSource; @@ -38,6 +39,11 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT Logger ourLog = LoggerFactory.getLogger(IWorkChunkStateTransitions.class); + @BeforeEach + default void before() { + getTestManager().enableMaintenanceRunner(false); + } + @ParameterizedTest @CsvSource({ "false, READY", @@ -89,7 +95,6 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT default void advanceJobStepAndUpdateChunkStatus_forGatedJob_updatesBothREADYAndQUEUEDChunks() { // setup getTestManager().disableWorkChunkMessageHandler(); - getTestManager().enableMaintenanceRunner(false); String state = """ 1|COMPLETED @@ -117,7 +122,6 @@ public interface IWorkChunkStateTransitions extends IWorkChunkCommon, WorkChunkT default void enqueueWorkChunkForProcessing_enqueuesOnlyREADYChunks() throws InterruptedException { // setup getTestManager().disableWorkChunkMessageHandler(); - getTestManager().enableMaintenanceRunner(false); StringBuilder sb = new StringBuilder(); // first step is always complete diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java index 6fb3b0e2787..3d1c8d8af30 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkStorageTests.java @@ -21,6 +21,7 @@ package ca.uhn.hapi.fhir.batch2.test; import ca.uhn.fhir.batch2.model.JobInstance; import ca.uhn.fhir.batch2.model.WorkChunk; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertNull; @@ -47,6 +48,11 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestConstants { + @BeforeEach + default void before() { + getTestManager().enableMaintenanceRunner(false); + } + @Test default void testStoreAndFetchWorkChunk_NoData() { JobInstance instance = createInstance(); @@ -69,8 +75,6 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC JobInstance instance = createInstance(); String instanceId = getTestManager().getSvc().storeNewInstance(instance); - getTestManager().enableMaintenanceRunner(false); - String id = getTestManager().storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 0, CHUNK_DATA, theGatedExecution); assertNotNull(id); @@ -81,7 +85,6 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC default void testNonGatedWorkChunkInReady_IsQueuedDuringMaintenance() throws InterruptedException { // setup int expectedCalls = 1; - getTestManager().enableMaintenanceRunner(false); PointcutLatch sendingLatch = getTestManager().disableWorkChunkMessageHandler(); sendingLatch.setExpectedCount(expectedCalls); String state = "1|READY,1|QUEUED"; @@ -107,7 +110,6 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC default void testStoreAndFetchWorkChunk_WithData() { // setup getTestManager().disableWorkChunkMessageHandler(); - getTestManager().enableMaintenanceRunner(false); JobDefinition jobDefinition = getTestManager().withJobDefinition(false); JobInstance instance = createInstance(); String instanceId = getTestManager().getSvc().storeNewInstance(instance); @@ -142,7 +144,6 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC // setup String state = "2|IN_PROGRESS,2|COMPLETED"; getTestManager().disableWorkChunkMessageHandler(); - getTestManager().enableMaintenanceRunner(false); JobDefinition jobDefinition = getTestManager().withJobDefinition(false); String instanceId = getTestManager().createAndStoreJobInstance(jobDefinition); @@ -172,7 +173,6 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC // setup String state = "1|IN_PROGRESS,1|ERRORED"; getTestManager().disableWorkChunkMessageHandler(); - getTestManager().enableMaintenanceRunner(false); JobDefinition jobDef = getTestManager().withJobDefinition(false); String instanceId = getTestManager().createAndStoreJobInstance(jobDef); JobMaintenanceStateInformation info = new JobMaintenanceStateInformation( @@ -214,7 +214,6 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC // setup String state = "1|IN_PROGRESS,1|FAILED"; getTestManager().disableWorkChunkMessageHandler(); - getTestManager().enableMaintenanceRunner(false); JobDefinition jobDef = getTestManager().withJobDefinition(false); String instanceId = getTestManager().createAndStoreJobInstance(jobDef); JobMaintenanceStateInformation info = new JobMaintenanceStateInformation( @@ -246,7 +245,6 @@ public interface IWorkChunkStorageTests extends IWorkChunkCommon, WorkChunkTestC 1|IN_PROGRESS,1|COMPLETED """; getTestManager().disableWorkChunkMessageHandler(); - getTestManager().enableMaintenanceRunner(false); JobDefinition jobDef = getTestManager().withJobDefinition(false); String instanceId = getTestManager().createAndStoreJobInstance(jobDef); JobMaintenanceStateInformation info = new JobMaintenanceStateInformation( From 2dc101bbc8ff8ce53cce3cb763f0e5cc382702d2 Mon Sep 17 00:00:00 2001 From: tyner Date: Fri, 12 Apr 2024 11:07:37 -0400 Subject: [PATCH 10/13] resolved review comments --- .../jpa/batch2/JpaJobPersistenceImplTest.java | 25 ++++++++++++------- .../batch2/test/IJobMaintenanceActions.java | 23 +++++++++++------ .../batch2/model/WorkChunkStatusEnum.java | 1 - 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index 6c9b2202221..f186ca79c45 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -374,7 +374,8 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { @Test public void testUpdateTime() { // Setup - JobInstance instance = createInstance(true, false); + boolean isGatedExecution = false; + JobInstance instance = createInstance(true, isGatedExecution); String instanceId = mySvc.storeNewInstance(instance); Date updateTime = runInTransaction(() -> new Date(findInstanceByIdOrThrow(instanceId).getUpdateTime().getTime())); @@ -392,10 +393,11 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { @Test public void advanceJobStepAndUpdateChunkStatus_forGatedJob_updatesCurrentStepAndChunkStatus() { // setup - JobInstance instance = createInstance(true, true); + boolean isGatedExecution = true; + JobInstance instance = createInstance(true, isGatedExecution); String instanceId = mySvc.storeNewInstance(instance); - String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); - String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); + String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, isGatedExecution); + String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, isGatedExecution); runInTransaction(() -> assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); @@ -416,10 +418,11 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { @Test public void advanceJobStepAndUpdateChunkStatus_whenAlreadyInTargetStep_DoesNotUpdateStepOrChunks() { // setup - JobInstance instance = createInstance(true, true); + boolean isGatedExecution = true; + JobInstance instance = createInstance(true, isGatedExecution); String instanceId = mySvc.storeNewInstance(instance); - String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); - String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, true); + String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, isGatedExecution); + String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, isGatedExecution); runInTransaction(() -> assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); @@ -447,7 +450,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { "false, READY, QUEUED", "true, GATE_WAITING, QUEUED" }) - public void testStoreAndFetchWorkChunk_withOrWithoutGatedExecution_createdAndTransitionToExpectedStatus(boolean theGatedExecution, WorkChunkStatusEnum theExpectedStatusOnCreate, WorkChunkStatusEnum theExpectedStatusAfterTransition) { + public void testStoreAndFetchWorkChunk_withOrWithoutGatedExecutionNoData_createdAndTransitionToExpectedStatus(boolean theGatedExecution, WorkChunkStatusEnum theExpectedStatusOnCreate, WorkChunkStatusEnum theExpectedStatusAfterTransition) { // setup JobInstance instance = createInstance(true, theGatedExecution); String instanceId = mySvc.storeNewInstance(instance); @@ -459,6 +462,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { runInTransaction(() -> assertEquals(theExpectedStatusAfterTransition, findChunkByIdOrThrow(id).getStatus())); WorkChunk chunk = mySvc.onWorkChunkDequeue(id).orElseThrow(IllegalArgumentException::new); + // assert null since we did not input any data when creating the chunks assertNull(chunk.getData()); } @@ -476,6 +480,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String secondChunkId = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, expectedSecondChunkData, isGatedExecution); runInTransaction(() -> { + // check chunks created in expected states assertEquals(WorkChunkStatusEnum.READY, findChunkByIdOrThrow(firstChunkId).getStatus()); assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(secondChunkId).getStatus()); }); @@ -483,6 +488,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { myBatch2JobHelper.runMaintenancePass(); runInTransaction(() -> { assertEquals(WorkChunkStatusEnum.QUEUED, findChunkByIdOrThrow(firstChunkId).getStatus()); + // maintenance should not affect chunks in step 2 assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(secondChunkId).getStatus()); }); @@ -499,6 +505,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { myBatch2JobHelper.runMaintenancePass(); runInTransaction(() -> { assertEquals(WorkChunkStatusEnum.COMPLETED, findChunkByIdOrThrow(firstChunkId).getStatus()); + // now that all chunks for step 1 is COMPLETED, should enqueue chunks in step 2 assertEquals(WorkChunkStatusEnum.QUEUED, findChunkByIdOrThrow(secondChunkId).getStatus()); }); @@ -579,7 +586,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { "false, READY, QUEUED", "true, GATE_WAITING, QUEUED" }) - public void testStoreAndFetchWorkChunk_WithData(boolean theGatedExecution, WorkChunkStatusEnum theExpectedCreatedStatus, WorkChunkStatusEnum theExpectedTransitionStatus) { + public void testStoreAndFetchWorkChunk_withOrWithoutGatedExecutionwithData_createdAndTransitionToExpectedStatus(boolean theGatedExecution, WorkChunkStatusEnum theExpectedCreatedStatus, WorkChunkStatusEnum theExpectedTransitionStatus) { JobInstance instance = createInstance(true, theGatedExecution); String instanceId = mySvc.storeNewInstance(instance); diff --git a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java index 5426fc54948..cb561ad87a1 100644 --- a/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java +++ b/hapi-fhir-storage-batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java @@ -1,6 +1,7 @@ package ca.uhn.hapi.fhir.batch2.test; import ca.uhn.fhir.batch2.model.JobDefinition; +import ca.uhn.fhir.batch2.model.JobInstance; import ca.uhn.hapi.fhir.batch2.test.support.JobMaintenanceStateInformation; import ca.uhn.test.concurrency.PointcutLatch; import org.junit.jupiter.api.BeforeEach; @@ -10,6 +11,8 @@ import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import static org.junit.jupiter.api.Assertions.assertEquals; + public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestConstants { Logger ourLog = LoggerFactory.getLogger(IJobMaintenanceActions.class); @@ -107,14 +110,14 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC 3|GATE_WAITING """ }) - default void testGatedStep2NotReady_stepNotAdvance(String theChunkState) throws InterruptedException { + default void testGatedStep2NotReady_stepNotAdvanceToStep3(String theChunkState) throws InterruptedException { // setup int expectedLatchCount = getLatchCountFromState(theChunkState); PointcutLatch sendingLatch = getTestManager().disableWorkChunkMessageHandler(); sendingLatch.setExpectedCount(expectedLatchCount); - JobMaintenanceStateInformation result = setupGatedWorkChunkTransitionTest(theChunkState, true); + JobMaintenanceStateInformation state = setupGatedWorkChunkTransitionTest(theChunkState, true); - getTestManager().createChunksInStates(result); + getTestManager().createChunksInStates(state); // test getTestManager().runMaintenancePass(); @@ -122,7 +125,8 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC // verify // nothing ever queued -> nothing ever sent to queue getTestManager().verifyWorkChunkMessageHandlerCalled(sendingLatch, expectedLatchCount); - verifyWorkChunkFinalStates(result); + assertEquals(SECOND_STEP_ID, getJobInstanceFromState(state).getCurrentGatedStepId()); + verifyWorkChunkFinalStates(state); } /** @@ -167,15 +171,16 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC // setup PointcutLatch sendingLatch = getTestManager().disableWorkChunkMessageHandler(); sendingLatch.setExpectedCount(2); - JobMaintenanceStateInformation result = setupGatedWorkChunkTransitionTest(theChunkState, true); - getTestManager().createChunksInStates(result); + JobMaintenanceStateInformation state = setupGatedWorkChunkTransitionTest(theChunkState, true); + getTestManager().createChunksInStates(state); // test getTestManager().runMaintenancePass(); // verify getTestManager().verifyWorkChunkMessageHandlerCalled(sendingLatch, 2); - verifyWorkChunkFinalStates(result); + assertEquals(LAST_STEP_ID, getJobInstanceFromState(state).getCurrentGatedStepId()); + verifyWorkChunkFinalStates(state); } @Test @@ -222,4 +227,8 @@ public interface IJobMaintenanceActions extends IWorkChunkCommon, WorkChunkTestC private void verifyWorkChunkFinalStates(JobMaintenanceStateInformation theStateInformation) { theStateInformation.verifyFinalStates(getTestManager().getSvc()); } + + private JobInstance getJobInstanceFromState(JobMaintenanceStateInformation state) { + return getTestManager().freshFetchJobInstance(state.getInstanceId()); + } } diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkStatusEnum.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkStatusEnum.java index 9b9d2517671..1c775b364cb 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkStatusEnum.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/model/WorkChunkStatusEnum.java @@ -31,7 +31,6 @@ import java.util.Set; * @see hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md */ public enum WorkChunkStatusEnum { - // wipmb For 6.8 Add WAITING for gated, and READY for in db, but not yet sent to channel. GATE_WAITING, READY, QUEUED, From 0569ddc81279d7bf3b9f316f75e6ebd51c1a73fe Mon Sep 17 00:00:00 2001 From: tyner Date: Fri, 12 Apr 2024 11:27:05 -0400 Subject: [PATCH 11/13] resolved review comments --- .../jpa/batch2/JpaJobPersistenceImplTest.java | 36 ++++++++++++------- .../maintenance/JobInstanceProcessor.java | 6 ---- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java index f186ca79c45..010b17c6739 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImplTest.java @@ -79,7 +79,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public class JpaJobPersistenceImplTest extends BaseJpaR4Test { public static final String JOB_DEFINITION_ID = "definition-id"; - public static final String TARGET_STEP_ID = TestJobDefinitionUtils.FIRST_STEP_ID; + public static final String FIRST_STEP_ID = TestJobDefinitionUtils.FIRST_STEP_ID; public static final String LAST_STEP_ID = TestJobDefinitionUtils.LAST_STEP_ID; public static final String DEF_CHUNK_ID = "definition-chunkId"; public static final String STEP_CHUNK_ID = TestJobDefinitionUtils.FIRST_STEP_ID; @@ -112,7 +112,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { JobInstance instance = createInstance(); String instanceId = mySvc.storeNewInstance(instance); for (int i = 0; i < 10; i++) { - storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, i, JsonUtil.serialize(new NdJsonFileJson().setNdJsonText("{}")), false); + storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, i, JsonUtil.serialize(new NdJsonFileJson().setNdJsonText("{}")), false); } // Execute @@ -247,8 +247,8 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { // Setup JobInstance instance = createInstance(); String instanceId = mySvc.storeNewInstance(instance); - storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA, false); - WorkChunkCreateEvent batchWorkChunk = new WorkChunkCreateEvent(JOB_DEFINITION_ID, JOB_DEF_VER, TARGET_STEP_ID, instanceId, 0, CHUNK_DATA, false); + storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 0, CHUNK_DATA, false); + WorkChunkCreateEvent batchWorkChunk = new WorkChunkCreateEvent(JOB_DEFINITION_ID, JOB_DEF_VER, FIRST_STEP_ID, instanceId, 0, CHUNK_DATA, false); String chunkId = mySvc.onWorkChunkCreate(batchWorkChunk); Optional byId = myWorkChunkRepository.findById(chunkId); Batch2WorkChunkEntity entity = byId.get(); @@ -399,7 +399,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, isGatedExecution); String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, isGatedExecution); - runInTransaction(() -> assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); + runInTransaction(() -> assertEquals(FIRST_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); // execute runInTransaction(() -> { @@ -424,11 +424,11 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String chunkIdSecondStep1 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, isGatedExecution); String chunkIdSecondStep2 = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, isGatedExecution); - runInTransaction(() -> assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); + runInTransaction(() -> assertEquals(FIRST_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId())); // execute runInTransaction(() -> { - boolean changed = mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, TARGET_STEP_ID); + boolean changed = mySvc.advanceJobStepAndUpdateChunkStatus(instanceId, FIRST_STEP_ID); assertFalse(changed); }); @@ -436,7 +436,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { runInTransaction(() -> { assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdSecondStep1).getStatus()); assertEquals(WorkChunkStatusEnum.GATE_WAITING, findChunkByIdOrThrow(chunkIdSecondStep2).getStatus()); - assertEquals(TARGET_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId()); + assertEquals(FIRST_STEP_ID, findInstanceByIdOrThrow(instanceId).getCurrentGatedStepId()); }); } @@ -456,6 +456,10 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String instanceId = mySvc.storeNewInstance(instance); // execute & verify + String firstChunkId = storeFirstWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 0, null); + // mark the first chunk as COMPLETED to allow step advance + runInTransaction(() -> myWorkChunkRepository.updateChunkStatus(firstChunkId, WorkChunkStatusEnum.COMPLETED, WorkChunkStatusEnum.READY)); + String id = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, null, theGatedExecution); runInTransaction(() -> assertEquals(theExpectedStatusOnCreate, findChunkByIdOrThrow(id).getStatus())); myBatch2JobHelper.runMaintenancePass(); @@ -476,7 +480,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { String instanceId = mySvc.storeNewInstance(instance); // execute & verify - String firstChunkId = storeFirstWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, expectedFirstChunkData); + String firstChunkId = storeFirstWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 0, expectedFirstChunkData); String secondChunkId = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, expectedSecondChunkData, isGatedExecution); runInTransaction(() -> { @@ -521,9 +525,9 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { JobInstance instance = createInstance(); String instanceId = mySvc.storeNewInstance(instance); - String queuedId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 0, "some data", isGatedExecution); - String erroredId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 1, "some more data", isGatedExecution); - String completedId = storeWorkChunk(JOB_DEFINITION_ID, TARGET_STEP_ID, instanceId, 2, "some more data", isGatedExecution); + String queuedId = storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 0, "some data", isGatedExecution); + String erroredId = storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 1, "some more data", isGatedExecution); + String completedId = storeWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 2, "some more data", isGatedExecution); mySvc.onWorkChunkDequeue(erroredId); WorkChunkErrorEvent parameters = new WorkChunkErrorEvent(erroredId, "Our error message"); @@ -547,7 +551,7 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { assertEquals(JOB_DEFINITION_ID, workChunk.getJobDefinitionId()); assertEquals(JOB_DEF_VER, workChunk.getJobDefinitionVersion()); assertEquals(instanceId, workChunk.getInstanceId()); - assertEquals(TARGET_STEP_ID, workChunk.getTargetStepId()); + assertEquals(FIRST_STEP_ID, workChunk.getTargetStepId()); assertEquals(0, workChunk.getSequence()); assertEquals(WorkChunkStatusEnum.READY, workChunk.getStatus()); @@ -587,9 +591,15 @@ public class JpaJobPersistenceImplTest extends BaseJpaR4Test { "true, GATE_WAITING, QUEUED" }) public void testStoreAndFetchWorkChunk_withOrWithoutGatedExecutionwithData_createdAndTransitionToExpectedStatus(boolean theGatedExecution, WorkChunkStatusEnum theExpectedCreatedStatus, WorkChunkStatusEnum theExpectedTransitionStatus) { + // setup JobInstance instance = createInstance(true, theGatedExecution); String instanceId = mySvc.storeNewInstance(instance); + // execute & verify + String firstChunkId = storeFirstWorkChunk(JOB_DEFINITION_ID, FIRST_STEP_ID, instanceId, 0, null); + // mark the first chunk as COMPLETED to allow step advance + runInTransaction(() -> myWorkChunkRepository.updateChunkStatus(firstChunkId, WorkChunkStatusEnum.COMPLETED, WorkChunkStatusEnum.READY)); + String id = storeWorkChunk(JOB_DEFINITION_ID, LAST_STEP_ID, instanceId, 0, CHUNK_DATA, theGatedExecution); assertNotNull(id); runInTransaction(() -> assertEquals(theExpectedCreatedStatus, findChunkByIdOrThrow(id).getStatus())); diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java index 0a7321a3629..62600e2f6c9 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java @@ -241,12 +241,6 @@ public class JobInstanceProcessor { Set workChunkStatuses = myJobPersistence.getDistinctWorkChunkStatesForJobAndStep( theInstance.getInstanceId(), currentGatedStepId); - if (workChunkStatuses.isEmpty()) { - // no work chunks = no output - // trivial to advance to next step - return true; - } - // all workchunks for the current step are in COMPLETED -> proceed. return workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.COMPLETED)); } From 11e4ef8319b802bea2a02b5e85d95f8e22e47e8b Mon Sep 17 00:00:00 2001 From: tyner Date: Fri, 12 Apr 2024 14:11:21 -0400 Subject: [PATCH 12/13] fixed bugs --- .../fhir/jpa/batch2/Batch2JobMaintenanceIT.java | 2 +- .../maintenance/JobInstanceProcessor.java | 17 ++++++++++++----- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/Batch2JobMaintenanceIT.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/Batch2JobMaintenanceIT.java index 7fb41a9f1eb..ec40941af65 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/Batch2JobMaintenanceIT.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/batch2/Batch2JobMaintenanceIT.java @@ -206,7 +206,7 @@ public class Batch2JobMaintenanceIT extends BaseJpaR4Test { @Nonnull private JobDefinition buildGatedJobDefinition(String theJobId, IJobStepWorker theFirstStep, IJobStepWorker theLastStep) { - return TestJobDefinitionUtils.buildJobDefinition( + return TestJobDefinitionUtils.buildGatedJobDefinition( theJobId, theFirstStep, theLastStep, diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java index 62600e2f6c9..5254ee10782 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java @@ -99,18 +99,19 @@ public class JobInstanceProcessor { cleanupInstance(theInstance); triggerGatedExecutions(theInstance, jobDefinition); + JobInstance updatedInstance = myJobPersistence.fetchInstance(theInstance.getInstanceId()).orElseThrow(); if (theInstance.hasGatedStep()) { JobWorkCursor jobWorkCursor = JobWorkCursor.fromJobDefinitionAndRequestedStepId( - jobDefinition, theInstance.getCurrentGatedStepId()); + jobDefinition, updatedInstance.getCurrentGatedStepId()); if (jobWorkCursor.isReductionStep()) { // Reduction step work chunks should never be sent to the queue but to its specific service instead. - triggerReductionStep(theInstance, jobWorkCursor); + triggerReductionStep(updatedInstance, jobWorkCursor); return; } } // enqueue the chunks as normal - enqueueReadyChunks(theInstance, jobDefinition); + enqueueReadyChunks(updatedInstance, jobDefinition); ourLog.debug("Finished job processing: {} - {}", myInstanceId, stopWatch); } @@ -241,6 +242,12 @@ public class JobInstanceProcessor { Set workChunkStatuses = myJobPersistence.getDistinctWorkChunkStatesForJobAndStep( theInstance.getInstanceId(), currentGatedStepId); + if (workChunkStatuses.isEmpty()) { + // no work chunks = no output + // trivial to advance to next step + return true; + } + // all workchunks for the current step are in COMPLETED -> proceed. return workChunkStatuses.equals(Set.of(WorkChunkStatusEnum.COMPLETED)); } @@ -337,11 +344,11 @@ public class JobInstanceProcessor { private void processChunksForNextGatedSteps(JobInstance theInstance, String nextStepId) { String instanceId = theInstance.getInstanceId(); List readyChunksForNextStep = - myProgressAccumulator.getChunkIdsWithStatus(instanceId, nextStepId, WorkChunkStatusEnum.READY); + myProgressAccumulator.getChunkIdsWithStatus(instanceId, nextStepId, WorkChunkStatusEnum.GATE_WAITING); int totalChunksForNextStep = myProgressAccumulator.getTotalChunkCountForInstanceAndStep(instanceId, nextStepId); if (totalChunksForNextStep != readyChunksForNextStep.size()) { ourLog.debug( - "Total ProgressAccumulator READY chunk count does not match READY chunk size! [instanceId={}, stepId={}, totalChunks={}, queuedChunks={}]", + "Total ProgressAccumulator GATE_WAITING chunk count does not match GATE_WAITING chunk size! [instanceId={}, stepId={}, totalChunks={}, queuedChunks={}]", instanceId, nextStepId, totalChunksForNextStep, From 8e76804d39712d5c9de5baf20db173e3452d409e Mon Sep 17 00:00:00 2001 From: tyner Date: Fri, 12 Apr 2024 14:11:53 -0400 Subject: [PATCH 13/13] spotless --- .../ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java index 5254ee10782..e70b7d91f05 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java @@ -99,7 +99,8 @@ public class JobInstanceProcessor { cleanupInstance(theInstance); triggerGatedExecutions(theInstance, jobDefinition); - JobInstance updatedInstance = myJobPersistence.fetchInstance(theInstance.getInstanceId()).orElseThrow(); + JobInstance updatedInstance = + myJobPersistence.fetchInstance(theInstance.getInstanceId()).orElseThrow(); if (theInstance.hasGatedStep()) { JobWorkCursor jobWorkCursor = JobWorkCursor.fromJobDefinitionAndRequestedStepId( jobDefinition, updatedInstance.getCurrentGatedStepId());