-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
5745 ready state batch2 #5764
base: master
Are you sure you want to change the base?
5745 ready state batch2 #5764
Conversation
Formatting check succeeded! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks pretty solid. I made a bunch of small suggestions but there is one must-fix, and one path that is too complicated:
- We can't fetch the whole chunk in the maintenance loop. Only the id.
- The nested internal ifs in the callback are too complicated. Let's figure out how to simplify those.
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md
Outdated
Show resolved
Hide resolved
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/introduction.md
Outdated
Show resolved
Hide resolved
hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/docs/server_jpa_batch/batch2_states.md
Outdated
Show resolved
Hide resolved
...resources/ca/uhn/hapi/fhir/changelog/7_2_0/5745-added-ready-state-to-batch2-work-chunks.yaml
Outdated
Show resolved
Hide resolved
hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java
Outdated
Show resolved
Hide resolved
hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java
Outdated
Show resolved
Hide resolved
hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java
Outdated
Show resolved
Hide resolved
hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java
Outdated
Show resolved
Hide resolved
...r-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobMaintenanceServiceImpl.java
Outdated
Show resolved
Hide resolved
...fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/coordinator/JobCoordinatorImplTest.java
Show resolved
Hide resolved
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPersistenceImpl.java
Outdated
Show resolved
Hide resolved
hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No show-stoppers.
I think the new interator class has already been written.
Love the new metadata view. Much more explicit that the old no-data path.
hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/entity/Batch2WorkChunkMetadataView.java
Show resolved
Hide resolved
hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/maintenance/JobInstanceProcessor.java
Outdated
Show resolved
Hide resolved
...batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java
Outdated
Show resolved
Hide resolved
...2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IWorkChunkErrorActionsTests.java
Outdated
Show resolved
Hide resolved
hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPersistence.java
Show resolved
Hide resolved
...batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java
Show resolved
Hide resolved
...batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java
Show resolved
Hide resolved
...batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java
Outdated
Show resolved
Hide resolved
...batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java
Show resolved
Hide resolved
...batch2-test-utilities/src/main/java/ca/uhn/hapi/fhir/batch2/test/IJobMaintenanceActions.java
Show resolved
Hide resolved
- 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
This reverts commit 32a00f4.
closes #5745