Address code review comments

This commit is contained in:
Martha 2024-07-05 16:33:50 -07:00
parent 8bfbc90315
commit 8f7a1aabf8
8 changed files with 22 additions and 16 deletions

View File

@ -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);
}
}

View File

@ -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;
}

View File

@ -27,7 +27,7 @@ public class JpaJobPartitionProviderTest {
@Mock
private IPartitionLookupSvc myPartitionLookupSvc;
@InjectMocks
private JpaPartitionProvider myJobPartitionProvider;
private JpaJobPartitionProvider myJobPartitionProvider;
@Test
public void getPartitions_requestSpecificPartition_returnsPartition() {

View File

@ -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);

View File

@ -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.
*/

View File

@ -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);
}
}

View File

@ -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;
}

View File

@ -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() {