Negative versions #3840

SergeyGaluzo · 2024-05-07T00:09:46Z

No description provided.

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Sql/Sprocs/GetResourceVersions.sql

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

EXPEkesheth · 2024-05-20T21:11:55Z

docs/BulkImport.md

@@ -105,7 +105,8 @@ Content-Type:application/fhir+json
 | Parameter Name      | Description | Card. |  Accepted values |
 | ----------- | ----------- | ----------- | ----------- |
 | inputFormat      | String representing the name of the data source format. Currently only FHIR NDJSON files are supported. | 1..1 | ```application/fhir+ndjson``` |
-| mode      | Import mode. Currently only initial load mode is supported. | 1..1 | For initial import use ```InitialLoad``` mode value. For incremental import mode use ```IncrementalLoad``` mode value. If no mode value is provided, IncrementalLoad mode value is considered by default. |
+| mode      | Import mode. | 0..1 | For initial import use ```InitialLoad``` mode value. For incremental import mode use ```IncrementalLoad``` mode value. If no mode value is provided, IncrementalLoad mode value is considered by default. |
+| allowNegativeVersions | Allows FHIR server assigning negative versions for out of order processed records with explicit lastUpdated value and no version specified. | 0..1 | To enable this feature pass true. By default it is false. |


Can we update the flag name to "resource-versionagnostic"? So when it is set to true, this feature is enabled.

This flag does not enable "version agnostic" import, because imports without input version are supported regardless of this flag. It just changes behavior of existing "version agnostic" functionality to create negative versions in case when input does not fit in contiguous space of positive versions existing in the database.

Current name directly addresses this additional functionality.

In future, we decided to got with character support for version. How will this flag consumed?

When we decide to support version as string, "version agnostic" imports without version conflicts will be supported without any special flags. This will be their default behavior as there will be not any version contiguity requirements. When and if this happens, this flag will not be needed and should be deprecated.

Ok looks good to me

EXPEkesheth · 2024-05-20T21:14:06Z

docs/BulkImport.md

@@ -105,7 +105,8 @@ Content-Type:application/fhir+json
 | Parameter Name      | Description | Card. |  Accepted values |
 | ----------- | ----------- | ----------- | ----------- |
 | inputFormat      | String representing the name of the data source format. Currently only FHIR NDJSON files are supported. | 1..1 | ```application/fhir+ndjson``` |
-| mode      | Import mode. Currently only initial load mode is supported. | 1..1 | For initial import use ```InitialLoad``` mode value. For incremental import mode use ```IncrementalLoad``` mode value. If no mode value is provided, IncrementalLoad mode value is considered by default. |
+| mode      | Import mode. | 0..1 | For initial import use ```InitialLoad``` mode value. For incremental import mode use ```IncrementalLoad``` mode value. If no mode value is provided, IncrementalLoad mode value is considered by default. |
+| allowNegativeVersions | Allows FHIR server assigning negative versions for out of order processed records with explicit lastUpdated value and no version specified. | 0..1 | To enable this feature pass true. By default it is false. |


Should the description state -" Ingests records in FHIR service based on lastUpdated value when no version is specified. In case of records ingested out of order, negative version values can be assigned.

"Ingests records in FHIR service based on lastUpdated value when no version is specified"
is not limited to this new flag. This behavior exists even without it. Only in cases when there is no "room" for incoming record in positive version space this flag changes behavior from "drop on the floor with conflict" to "ingest with negative version".

We should make this clear, so customers are not confused.

If current description is not specific enough, I suggest
"
Allows FHIR server assigning negative versions for resource records with explicit lastUpdated value and no version specified when input does not fit in contiguous space of positive versions existing in the store.
"

mikaelweave

A couple small comments but approved 🦾

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

mikaelweave · 2024-05-20T22:16:55Z

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

@@ -379,7 +378,7 @@ await using (new Timer(async _ => await _sqlStoreClient.MergeResourcesPutTransac
            return results;
        }

-        internal async Task<IReadOnlyList<string>> ImportResourcesAsync(IReadOnlyList<ImportResource> resources, ImportMode importMode, CancellationToken cancellationToken)
+        internal async Task<IReadOnlyList<string>> ImportResourcesAsync(IReadOnlyList<ImportResource> resources, ImportMode importMode, bool allowNegativeVersions, CancellationToken cancellationToken)


Note: If we enhance this in the future with more logic, we should evaluate breaking this single unit into multiple units as long units are harder to debug/enhance without breaking.

mikaelweave · 2024-05-20T22:34:10Z

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

+            var history = await _client.SearchAsync($"Patient/{id}/_history");
+            Assert.Equal(2, history.Resource.Entry.Count);
+            Assert.Equal("1", history.Resource.Entry[0].Resource.VersionId);
+            Assert.Equal("-1", history.Resource.Entry[1].Resource.VersionId);


@SergeyGaluzo - should this be true even if the content of the resource is the same (except for explicit vs implicit last updated)? What would the behavior be if these resources were imported "in order" or "not late"?

SergeyGaluzo · 2024-05-21T00:49:38Z

@mikaelweave in reference to #3840 (comment)

When customer specifies last updated, he/she takes control over versioning. Only if last updated is not specified, we can do checks on data content. If we do opposite, we can run into the problem, when according to current database state a record does not need to be written, but if other late arrivals come, it makes this decision invalid.

mikaelweave · 2024-05-21T03:19:24Z

@mikaelweave in reference to #3840 (comment)

When customer specifies last updated, he/she takes control over versioning. Only if last updated is not specified, we can do checks on data content. If we do opposite, we can run into the problem, when according to current database state a record does not need to be written, but if other late arrivals come, it makes this decision invalid.

I understand - if resources are imported specifying last updated, then we encounter scenarios where this is required. Since we import latest first (by last updated) in a batch, the latest one here takes version number 1.

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs

mikaelweave

small NIT but looks great 😄

mikaelweave · 2024-05-28T20:56:05Z

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

@@ -182,6 +182,151 @@ public async Task GivenIncrementalLoad_80KSurrogateIds_BadRequestIsReturned()
            Assert.Contains(ImportProcessingJob.SurrogateIdsErrorMessage, await message.Content.ReadAsStringAsync());
        }

+        [Fact]
+        public async Task GivenIncrementalLoad_MultipleImportsWithSameLastUpdatedAndExplicitVersion()


NIT - test title doesn't match pattern. Should be
GivenIncrementalLoad_MultipleImportsWithSameLastUpdatedAndExplicitVersion_ResourceReloaded or similar.

Same for test below this one.

SergeyGaluzo added 5 commits May 1, 2024 22:17

conflict retries test

bcafccd

extra dedup plus tests

66a9556

bad request

1ad1cd2

better date

5be648e

more import deduping

26c6408

SergeyGaluzo added Enhancement Enhancement on existing functionality. Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Schema Version unchanged labels May 7, 2024

SergeyGaluzo added this to the S138 milestone May 7, 2024

SergeyGaluzo requested a review from a team as a code owner May 7, 2024 00:09

SergeyGaluzo added 4 commits May 6, 2024 21:47

80

7359c24

80

eeef298

dedup against db

54ba038

Allow negative versions parameter

d01cce8

SergeyGaluzo changed the title ~~[Do not review] Users/sergal/importdedupping~~ Negative versions May 8, 2024

Merge from main + removed flaky test header

2fab13e

github-advanced-security bot found potential problems May 8, 2024

View reviewed changes

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs Fixed Show fixed Hide fixed

SergeyGaluzo added 7 commits May 13, 2024 18:43

better stored proc

5c60e0d

comment

0346ef6

81

1e9a13f

merge

f10b135

merge

66b014e

item group back

9adee04

-CR

8a9a031

github-advanced-security bot found potential problems May 14, 2024

View reviewed changes

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs Fixed Show fixed Hide fixed

SergeyGaluzo added 2 commits May 14, 2024 08:04

81

877d722

81

7f8c550

LTA-Thinking reviewed May 14, 2024

View reviewed changes

src/Microsoft.Health.Fhir.SqlServer/Features/Schema/Sql/Sprocs/GetResourceVersions.sql Show resolved Hide resolved

set version on create is back

b628b97

LTA-Thinking reviewed May 14, 2024

View reviewed changes

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

EXPEkesheth reviewed May 20, 2024

View reviewed changes

mikaelweave previously approved these changes May 20, 2024

View reviewed changes

EXPEkesheth previously approved these changes May 21, 2024

View reviewed changes

SergeyGaluzo added 3 commits May 21, 2024 16:32

Description change

b687029

Added test 1-8-9-10

46ac216

move to loaded

b438454

SergeyGaluzo dismissed stale reviews from EXPEkesheth, mikaelweave, and LTA-Thinking via b438454 May 23, 2024 02:34

SergeyGaluzo added 9 commits May 23, 2024 08:40

correct date comparison

5e54cac

Add milliseconds truncation logic to import parser

7149098

Add truncate to millisecond to the tests

b4966e7

Removed filter on negative version in parser

ad42b46

_

acb70e7

negatives are history and no conflict.

2aae074

preserve offset

3f2651e

corrections for negative versions

09e21ef

Special logic to load negatives

e876807

github-advanced-security bot found potential problems May 26, 2024

View reviewed changes

src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs Dismissed Show dismissed Hide dismissed

Latest negative test

fd4a40a

microsoft deleted a comment from azure-pipelines bot May 28, 2024

SergeyGaluzo enabled auto-merge (squash) May 28, 2024 14:38

SergeyGaluzo disabled auto-merge May 28, 2024 15:00

Ignore input negatives if not allowed

822b152

LTA-Thinking approved these changes May 28, 2024

View reviewed changes

mikaelweave approved these changes May 28, 2024

View reviewed changes

SergeyGaluzo merged commit 8968ae1 into main May 28, 2024
46 of 47 checks passed

SergeyGaluzo deleted the users/sergal/importdedupping branch May 28, 2024 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Negative versions #3840

Negative versions #3840

SergeyGaluzo commented May 7, 2024

EXPEkesheth May 20, 2024

SergeyGaluzo May 21, 2024 •

edited

EXPEkesheth May 21, 2024

SergeyGaluzo May 21, 2024 •

edited

EXPEkesheth May 21, 2024

EXPEkesheth May 20, 2024

SergeyGaluzo May 21, 2024 •

edited

mikaelweave left a comment

mikaelweave May 20, 2024

mikaelweave May 20, 2024

SergeyGaluzo commented May 21, 2024

mikaelweave commented May 21, 2024 •

edited

mikaelweave left a comment

mikaelweave May 28, 2024

Negative versions #3840

Negative versions #3840

Conversation

SergeyGaluzo commented May 7, 2024

EXPEkesheth May 20, 2024

Choose a reason for hiding this comment

SergeyGaluzo May 21, 2024 • edited

Choose a reason for hiding this comment

EXPEkesheth May 21, 2024

Choose a reason for hiding this comment

SergeyGaluzo May 21, 2024 • edited

Choose a reason for hiding this comment

EXPEkesheth May 21, 2024

Choose a reason for hiding this comment

EXPEkesheth May 20, 2024

Choose a reason for hiding this comment

SergeyGaluzo May 21, 2024 • edited

Choose a reason for hiding this comment

mikaelweave left a comment

Choose a reason for hiding this comment

mikaelweave May 20, 2024

Choose a reason for hiding this comment

mikaelweave May 20, 2024

Choose a reason for hiding this comment

SergeyGaluzo commented May 21, 2024

mikaelweave commented May 21, 2024 • edited

mikaelweave left a comment

Choose a reason for hiding this comment

mikaelweave May 28, 2024

Choose a reason for hiding this comment

SergeyGaluzo May 21, 2024 •

edited

SergeyGaluzo May 21, 2024 •

edited

SergeyGaluzo May 21, 2024 •

edited

mikaelweave commented May 21, 2024 •

edited