Reindexes rest of batch if one resource fails #2118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rotodd
commented
Jul 26, 2021
Comment on lines
95
to
111
| DECLARE @resourcesNotInDatabase int | ||
| SET @resourcesNotInDatabase = (SELECT COUNT(*) FROM @computedValues WHERE ResourceSurrogateId IS NULL) | ||
|
|
||
| IF (@resourcesNotInDatabase > 0) BEGIN | ||
| -- We can't reindex resources that do not exist | ||
| DELETE FROM @computedValues | ||
| WHERE ResourceSurrogateId IS NULL | ||
| END | ||
|
|
||
| DECLARE @versionDiff int | ||
| SET @versionDiff = (SELECT COUNT(*) FROM @computedValues WHERE VersionProvided IS NOT NULL AND VersionProvided <> VersionInDatabase) | ||
|
|
||
| IF (@versionDiff > 0) BEGIN | ||
| -- Don't reindex resources that have outdated versions | ||
| DELETE FROM @computedValues | ||
| WHERE VersionProvided IS NOT NULL AND VersionProvided <> VersionInDatabase | ||
| END |
There was a problem hiding this comment.
Instead of throwing exceptions here, we simply remove the resources from @computedValues, the table of resources to be reindexed.
rotodd
commented
Jul 26, 2021
Comment on lines
351
to
352
| // TODO: Return total number of successfully reindexed resources. | ||
| int numberOfDeletedResources = sqlDataReader.GetInt32(1); |
|
This pull request introduces 1 alert when merging 5d8b004 into 2f0d55d - view on LGTM.com new alerts:
|
Ivanidzo4ka
reviewed
Jul 30, 2021
Ivanidzo4ka
reviewed
Jul 30, 2021
| var issue = new OperationOutcomeIssue( | ||
| OperationOutcomeConstants.IssueSeverity.Error, | ||
| OperationOutcomeConstants.IssueType.Exception, | ||
| ex.Message); |
There was a problem hiding this comment.
Good point. I am currently using this to pass an action item to the user on PreconditionFailed exceptions, but I can see how this might lead to us sending info to the user that wouldn't be helpful. Working on finding a nice fix now.
There was a problem hiding this comment.
Added an extra catch block in 52ea697.
Ivanidzo4ka
reviewed
Jul 30, 2021
src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/CosmosFhirDataStore.cs
Outdated
Show resolved
Hide resolved
Ivanidzo4ka
reviewed
Jul 30, 2021
src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs
Outdated
Show resolved
Hide resolved
Ivanidzo4ka
reviewed
Jul 30, 2021
src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlServerFhirDataStore.cs
Outdated
Show resolved
Hide resolved
feordin
reviewed
Jul 30, 2021
src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/CosmosFhirDataStore.cs
Show resolved
Hide resolved
|
/azp run |
|
Pull request contains merge conflicts. |
Ivanidzo4ka
reviewed
Aug 4, 2021
| -- @tokenNumberNumberCompositeSearchParams | ||
| -- * Extracted token$number$number search params | ||
| -- | ||
| CREATE OR ALTER PROCEDURE dbo.ReindexResource |
There was a problem hiding this comment.
May I ask why it doesn't upgrade version?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Prior to this PR, when one resource failed to reindex due to a versioning conflict, the entire batch would fail.
Now, the remaining resources in the batch are reindexed. The query will be retried, fetching the most recent version of the failed resource from the database. In the rare scenario where all consecutive retry attempts fail, the reindexing operation will fail with the following error message:
Related issues
Addresses AB#79925
Testing
Next steps
When one or more resources in a batch initially fail to reindex but succeed on later attempts, the reindex job progress does not pick up the resources that succeeded on the first attempt. I created AB#83909 to track this.
FHIR Team Checklist
Semver Change (docs)
Patch (new schema version)