Fix Cancel Import Request #4702

abiisnn · 2024-10-25T19:21:22Z

Description

Currently, the Orchestrator job completes before the processing jobs are completed, it will incorrectly not allow cancel of an import because it thinks the job is done. This is incorrect as the Orchestrator is done but not the child Processing Jobs,.

This PR is fixing this issue, now we check all jobs to check the status of all jobs.

Related issues

Addresses issue #124093

Testing

Added a test to make sure it first returns Accepted status to cancel request, which means the import job has been cancelled, and then make sure we received Conflict status if we have already cancelled the job.

The existing test was making sure that the cancel operation works if all jobs have already completed (with just one patient) it was happening.

FHIR Team Checklist

Update the title of the PR to be succinct and less than 65 characters
Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
CI is green before merge
Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs

src/Microsoft.Health.Fhir.Core/Features/Operations/Import/CancelImportRequestHandler.cs

abiisnn

/azp run

abiisnn · 2024-10-31T19:45:26Z

...oft.Health.Fhir.Core.UnitTests/Features/Operations/Import/CancelImportRequestHandlerTests.cs

@@ -66,14 +66,26 @@ public async Task GivenAFhirMediator_WhenCancelingExistingBulkImportJobThatHasAl
            Assert.Equal(HttpStatusCode.Conflict, operationFailedException.ResponseStatusCode);
        }

+        [Theory]
+        [InlineData(JobStatus.Cancelled)]
+        public async Task GivenAFhirMediator_WhenCancelingExistingBulkImportJobThatHasAlreadyBeenCanceled_ThenAcceptedStatusCodeShouldBeReturned(JobStatus taskStatus)


This is new behavior, because if a tastk has been already canceled, then we will return accepted because we will continue trying to cancel.

A change in behavior may require giving customers 1 month notice before deploying to prod. We should check w./ PM team

abiisnn · 2024-10-31T19:46:59Z

src/Microsoft.Health.Fhir.Core/Features/Operations/Import/CancelImportRequestHandler.cs

            {
                throw new ResourceNotFoundException(string.Format(Core.Resources.ImportJobNotFound, request.JobId));
            }

-            if (jobInfo.Status == JobManagement.JobStatus.Completed || jobInfo.Status == JobManagement.JobStatus.Cancelled || jobInfo.Status == JobManagement.JobStatus.Failed)


Please check that now we are not returning CONFLICT if the job has been already cancelled.

Now, if the job has been already cancelled, we will retry cancel, so we will return ACCEPTED, as Sergey suggested.

just need to check w/ PM on behavior change

...oft.Health.Fhir.Core.UnitTests/Features/Operations/Import/CancelImportRequestHandlerTests.cs

SergeyGaluzo · 2024-10-31T20:55:19Z

I don't think jobs == null condition needs to be checked.

SergeyGaluzo · 2024-10-31T20:56:11Z

I think it would be easier to use Linq instead of foreach.

mikaelweave

approved with some suggestions

mikaelweave · 2024-11-01T17:35:10Z

...oft.Health.Fhir.Core.UnitTests/Features/Operations/Import/CancelImportRequestHandlerTests.cs

@@ -66,14 +66,26 @@ public async Task GivenAFhirMediator_WhenCancelingExistingBulkImportJobThatHasAl
            Assert.Equal(HttpStatusCode.Conflict, operationFailedException.ResponseStatusCode);
        }

+        [Theory]
+        [InlineData(JobStatus.Cancelled)]
+        public async Task GivenAFhirMediator_WhenCancelingExistingBulkImportJobThatHasAlreadyBeenCanceled_ThenAcceptedStatusCodeShouldBeReturned(JobStatus taskStatus)


A change in behavior may require giving customers 1 month notice before deploying to prod. We should check w./ PM team

mikaelweave · 2024-11-01T17:37:17Z

src/Microsoft.Health.Fhir.Core/Features/Operations/Import/CancelImportRequestHandler.cs

            {
                throw new ResourceNotFoundException(string.Format(Core.Resources.ImportJobNotFound, request.JobId));
            }

-            if (jobInfo.Status == JobManagement.JobStatus.Completed || jobInfo.Status == JobManagement.JobStatus.Cancelled || jobInfo.Status == JobManagement.JobStatus.Failed)


just need to check w/ PM on behavior change

mikaelweave · 2024-11-01T17:45:01Z

test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs

+            Uri checkLocation = await ImportTestHelper.CreateImportTaskAsync(_client, request);
+
+            // Wait for import job to complete
+            await Task.Delay(TimeSpan.FromSeconds(10));


I worry this could create a flake test in the future.

Could we check the status of the import before moving forward in the test?

We could even loop and wait again if it's not complete (up to 5 times or so).

Great idea!

abiisnn

/azp run

abiisnn · 2024-11-01T19:32:22Z

test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs

+            Uri checkLocation = await ImportTestHelper.CreateImportTaskAsync(_client, request);
+
+            // Wait for import job to complete
+            await Task.Delay(TimeSpan.FromSeconds(10));


Great idea!

abiisnn · 2024-11-04T18:00:10Z

/azp run

azure-pipelines · 2024-11-04T18:00:26Z

Azure Pipelines successfully started running 1 pipeline(s).

Add test to fix later

baa4c2a

mikaelweave reviewed Oct 25, 2024

View reviewed changes

test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Import/ImportTests.cs Outdated Show resolved Hide resolved

Fix cancel

b2885be

abiisnn commented Oct 30, 2024

View reviewed changes

src/Microsoft.Health.Fhir.Core/Features/Operations/Import/CancelImportRequestHandler.cs Show resolved Hide resolved

abiisnn added 3 commits October 30, 2024 13:52

Add variable for better undestanding

4ba5c69

TYPO

9a5557b

Fix typo and add comments to first text

72bd795

abiisnn added this to the S153 milestone Oct 30, 2024

abiisnn added Bug Bug bug bug. Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Open source This change is only relevant to the OSS code or release. Schema Version unchanged labels Oct 30, 2024

abiisnn marked this pull request as ready for review October 30, 2024 21:22

abiisnn requested a review from a team as a code owner October 30, 2024 21:22

abiisnn commented Oct 30, 2024

View reviewed changes

Update to make sure we can continue cancel

af6991f

abiisnn commented Oct 31, 2024

View reviewed changes

github-advanced-security bot found potential problems Oct 31, 2024

View reviewed changes

...oft.Health.Fhir.Core.UnitTests/Features/Operations/Import/CancelImportRequestHandlerTests.cs Fixed Show fixed Hide fixed

delete variable

fcbd679

mikaelweave previously approved these changes Nov 1, 2024

View reviewed changes

Do not check if jobs are null

cce80ca

abiisnn dismissed mikaelweave’s stale review via cce80ca November 1, 2024 18:10

Avoid flaky test

b3b105e

abiisnn changed the title ~~Fix CancelImport request~~ Fix Cancel Import Request Nov 1, 2024

mikaelweave approved these changes Nov 1, 2024

View reviewed changes

abiisnn commented Nov 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Cancel Import Request #4702

Fix Cancel Import Request #4702

abiisnn commented Oct 25, 2024 •

edited

Loading

abiisnn left a comment

abiisnn Oct 31, 2024

mikaelweave Nov 1, 2024

abiisnn Oct 31, 2024 •

edited

Loading

mikaelweave Nov 1, 2024

SergeyGaluzo commented Oct 31, 2024

SergeyGaluzo commented Oct 31, 2024

mikaelweave left a comment

mikaelweave Nov 1, 2024

mikaelweave Nov 1, 2024

mikaelweave Nov 1, 2024

abiisnn Nov 1, 2024

abiisnn left a comment

abiisnn Nov 1, 2024

abiisnn commented Nov 4, 2024

azure-pipelines bot commented Nov 4, 2024

Fix Cancel Import Request #4702

Are you sure you want to change the base?

Fix Cancel Import Request #4702

Conversation

abiisnn commented Oct 25, 2024 • edited Loading

Description

Related issues

Testing

FHIR Team Checklist

Semver Change (docs)

abiisnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abiisnn Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergeyGaluzo commented Oct 31, 2024

SergeyGaluzo commented Oct 31, 2024

mikaelweave left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abiisnn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abiisnn commented Nov 4, 2024

azure-pipelines bot commented Nov 4, 2024

abiisnn commented Oct 25, 2024 •

edited

Loading

abiisnn Oct 31, 2024 •

edited

Loading