From 8f7a1aabf8e5ab77ba3411a40821162af56ee38e Mon Sep 17 00:00:00 2001 From: Martha Date: Fri, 5 Jul 2024 16:33:50 -0700 Subject: [PATCH] Address code review comments --- .../ca/uhn/fhir/jpa/batch2/JpaBatch2Config.java | 2 +- ...Provider.java => JpaJobPartitionProvider.java} | 15 +++++++++------ .../jpa/batch2/JpaJobPartitionProviderTest.java | 2 +- .../batch2/jobs/imprt/BulkDataImportProvider.java | 1 - .../fhir/batch2/api/IJobPartitionProvider.java | 3 ++- .../uhn/fhir/batch2/config/BaseBatch2Config.java | 4 ++-- ...vider.java => SimpleJobPartitionProvider.java} | 7 +++++-- ...t.java => SimpleJobPartitionProviderTest.java} | 4 ++-- 8 files changed, 22 insertions(+), 16 deletions(-) rename hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/{JpaPartitionProvider.java => JpaJobPartitionProvider.java} (67%) rename hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/{JobPartitionProvider.java => SimpleJobPartitionProvider.java} (71%) rename hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/coordinator/{JobPartitionProviderTest.java => SimpleJobPartitionProviderTest.java} (93%) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaBatch2Config.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaBatch2Config.java index 3cab50228e1..d8a97947837 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaBatch2Config.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaBatch2Config.java @@ -59,6 +59,6 @@ public class JpaBatch2Config extends BaseBatch2Config { @Bean public IJobPartitionProvider jobPartitionProvider( IRequestPartitionHelperSvc theRequestPartitionHelperSvc, IPartitionLookupSvc thePartitionLookupSvc) { - return new JpaPartitionProvider(theRequestPartitionHelperSvc, thePartitionLookupSvc); + return new JpaJobPartitionProvider(theRequestPartitionHelperSvc, thePartitionLookupSvc); } } diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaPartitionProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPartitionProvider.java similarity index 67% rename from hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaPartitionProvider.java rename to hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPartitionProvider.java index d52806cf46b..8f5ea0919cc 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaPartitionProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/batch2/JpaJobPartitionProvider.java @@ -1,6 +1,6 @@ package ca.uhn.fhir.jpa.batch2; -import ca.uhn.fhir.batch2.coordinator.JobPartitionProvider; +import ca.uhn.fhir.batch2.api.IJobPartitionProvider; import ca.uhn.fhir.interceptor.model.RequestPartitionId; import ca.uhn.fhir.jpa.entity.PartitionEntity; import ca.uhn.fhir.jpa.partition.IPartitionLookupSvc; @@ -11,15 +11,18 @@ import java.util.List; import java.util.stream.Collectors; /** - * The default implementation, which uses {@link IRequestPartitionHelperSvc} and {@link IPartitionLookupSvc} to compute the partition to run a batch2 job. - * The ladder will be used to handle cases when the job is configured to run against all partitions (bulk system operation). + * The default JPA implementation, which uses {@link IRequestPartitionHelperSvc} and {@link IPartitionLookupSvc} + * to compute the partition to run a batch2 job. + * The latter will be used to handle cases when the job is configured to run against all partitions + * (bulk system operation) and will return the actual list with all the configured partitions. */ -public class JpaPartitionProvider extends JobPartitionProvider { +public class JpaJobPartitionProvider implements IJobPartitionProvider { + protected final IRequestPartitionHelperSvc myRequestPartitionHelperSvc; private final IPartitionLookupSvc myPartitionLookupSvc; - public JpaPartitionProvider( + public JpaJobPartitionProvider( IRequestPartitionHelperSvc theRequestPartitionHelperSvc, IPartitionLookupSvc thePartitionLookupSvc) { - super(theRequestPartitionHelperSvc); + myRequestPartitionHelperSvc = theRequestPartitionHelperSvc; myPartitionLookupSvc = thePartitionLookupSvc; } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPartitionProviderTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPartitionProviderTest.java index a480d09dcc2..8540511959c 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPartitionProviderTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/batch2/JpaJobPartitionProviderTest.java @@ -27,7 +27,7 @@ public class JpaJobPartitionProviderTest { @Mock private IPartitionLookupSvc myPartitionLookupSvc; @InjectMocks - private JpaPartitionProvider myJobPartitionProvider; + private JpaJobPartitionProvider myJobPartitionProvider; @Test public void getPartitions_requestSpecificPartition_returnsPartition() { diff --git a/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/BulkDataImportProvider.java b/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/BulkDataImportProvider.java index d11df7d5dbe..16c27ced53a 100644 --- a/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/BulkDataImportProvider.java +++ b/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/BulkDataImportProvider.java @@ -159,7 +159,6 @@ public class BulkDataImportProvider { RequestPartitionId partitionId = myRequestPartitionHelperService.determineReadPartitionForRequestForServerOperation( theRequestDetails, JpaConstants.OPERATION_IMPORT); - // TODO MM: I believe this is already checked as part of myRequestPartitionHelperService.validateHasPartitionPermissions(theRequestDetails, "Binary", partitionId); jobParameters.setPartitionId(partitionId); diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPartitionProvider.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPartitionProvider.java index 3bd0a6bb2ee..20d8716d5e5 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPartitionProvider.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/api/IJobPartitionProvider.java @@ -7,7 +7,8 @@ import java.util.List; /** * Provides the list of partitions that a job should run against. - * TODO MM: Consider moving UrlPartitioner calls to this class once other operations need to be MegaScale enabled. + * TODO MM: Consider moving UrlPartitioner calls to this class once other batch operations need to support running + * across all partitions on a multitenant FHIR server. * That way all partitioning related logic exists only here for batch jobs. * After that PartitionedUrl#myRequestPartitionId can be marked as deprecated. */ diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/config/BaseBatch2Config.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/config/BaseBatch2Config.java index d403ee0f287..2dbc531df48 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/config/BaseBatch2Config.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/config/BaseBatch2Config.java @@ -27,8 +27,8 @@ import ca.uhn.fhir.batch2.api.IReductionStepExecutorService; import ca.uhn.fhir.batch2.channel.BatchJobSender; import ca.uhn.fhir.batch2.coordinator.JobCoordinatorImpl; import ca.uhn.fhir.batch2.coordinator.JobDefinitionRegistry; -import ca.uhn.fhir.batch2.coordinator.JobPartitionProvider; import ca.uhn.fhir.batch2.coordinator.ReductionStepExecutorServiceImpl; +import ca.uhn.fhir.batch2.coordinator.SimpleJobPartitionProvider; import ca.uhn.fhir.batch2.coordinator.WorkChunkProcessor; import ca.uhn.fhir.batch2.maintenance.JobMaintenanceServiceImpl; import ca.uhn.fhir.batch2.model.JobWorkNotificationJsonMessage; @@ -145,6 +145,6 @@ public abstract class BaseBatch2Config { @Bean public IJobPartitionProvider jobPartitionProvider(IRequestPartitionHelperSvc theRequestPartitionHelperSvc) { - return new JobPartitionProvider(theRequestPartitionHelperSvc); + return new SimpleJobPartitionProvider(theRequestPartitionHelperSvc); } } diff --git a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/JobPartitionProvider.java b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/SimpleJobPartitionProvider.java similarity index 71% rename from hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/JobPartitionProvider.java rename to hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/SimpleJobPartitionProvider.java index c129ce0c38e..8b57c5ee0b1 100644 --- a/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/JobPartitionProvider.java +++ b/hapi-fhir-storage-batch2/src/main/java/ca/uhn/fhir/batch2/coordinator/SimpleJobPartitionProvider.java @@ -7,10 +7,13 @@ import ca.uhn.fhir.rest.api.server.RequestDetails; import java.util.List; -public class JobPartitionProvider implements IJobPartitionProvider { +/** + * Basic implementation which provides the partition list for a certain request which is composed of a single partition. + */ +public class SimpleJobPartitionProvider implements IJobPartitionProvider { protected final IRequestPartitionHelperSvc myRequestPartitionHelperSvc; - public JobPartitionProvider(IRequestPartitionHelperSvc theRequestPartitionHelperSvc) { + public SimpleJobPartitionProvider(IRequestPartitionHelperSvc theRequestPartitionHelperSvc) { myRequestPartitionHelperSvc = theRequestPartitionHelperSvc; } diff --git a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/coordinator/JobPartitionProviderTest.java b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/coordinator/SimpleJobPartitionProviderTest.java similarity index 93% rename from hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/coordinator/JobPartitionProviderTest.java rename to hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/coordinator/SimpleJobPartitionProviderTest.java index 2a65f9050d4..df4b716db54 100644 --- a/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/coordinator/JobPartitionProviderTest.java +++ b/hapi-fhir-storage-batch2/src/test/java/ca/uhn/fhir/batch2/coordinator/SimpleJobPartitionProviderTest.java @@ -17,11 +17,11 @@ import java.util.List; import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) -public class JobPartitionProviderTest { +public class SimpleJobPartitionProviderTest { @Mock private IRequestPartitionHelperSvc myRequestPartitionHelperSvc; @InjectMocks - private JobPartitionProvider myJobPartitionProvider; + private SimpleJobPartitionProvider myJobPartitionProvider; @Test public void getPartitions_requestSpecificPartition_returnsPartition() {