Skip to content

MergeResources concurrent call return 429 #5029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

MergeResources concurrent call return 429 #5029

wants to merge 5 commits into from
+35 −37

Conversation

rbans96
Copy link
Contributor

@rbans96 rbans96 commented Jun 19, 2025

Description

Raise 429 errors if MergeResources concurrent calls is above optimal. Also add retryAfter header to suggest the backoff.

Related issues

Addresses [issue #].

Testing

Describe how this change was tested.

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.
  • When changing or adding behavior, if your code modifies the system design or changes design assumptions, please create and include an ADR.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@rbans96 rbans96 requested a review from a team as a code owner June 19, 2025 16:27
@rbans96 rbans96 added the Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs label Jun 19, 2025
@rbans96 rbans96 added this to the CY25Q2/2Wk06 milestone Jun 19, 2025
@rbans96 rbans96 added the Enhancement Enhancement on existing functionality. label Jun 19, 2025
@rbans96 rbans96 changed the title Personal/ribans/mergeresources return 429 MergeResources concurrent call return 429 Jun 19, 2025
@rbans96 rbans96 force-pushed the personal/ribans/mergeresources-return-429 branch from cf421f3 to 3549127 Compare June 19, 2025 18:59
github-advanced-security[bot]
github-advanced-security bot found potential problems Jun 19, 2025
View reviewed changes
src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlStoreClient.cs Outdated
Comment on lines 97 to 100
catch (Exception ex)
{
_logger.LogError(ex, "Unexpected error in SQL adaptive throttling sampling loop.");
}

Check notice

Code scanning / CodeQL

Generic catch clause

Generic catch clause.

Copilot Autofix

AI 1 day ago

To fix the issue, replace the generic catch (Exception ex) block with specific catch blocks for anticipated exceptions, such as TaskCanceledException and other exceptions that are expected in this context. For unexpected exceptions, rethrow them after logging to ensure they are not silently swallowed. This ensures that only known issues are handled, while unexpected ones are propagated for proper handling.

Suggested changeset 1
src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlStoreClient.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlStoreClient.cs b/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlStoreClient.cs
--- a/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlStoreClient.cs
+++ b/src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlStoreClient.cs
@@ -96,2 +96,6 @@
                 }
+                catch (SqlException sqlEx)
+                {
+                    _logger.LogError(sqlEx, "SQL error in adaptive throttling sampling loop.");
+                }
                 catch (Exception ex)
@@ -99,2 +103,3 @@
                     _logger.LogError(ex, "Unexpected error in SQL adaptive throttling sampling loop.");
+                    throw;
                 }
EOF
@@ -96,2 +96,6 @@
}
catch (SqlException sqlEx)
{
_logger.LogError(sqlEx, "SQL error in adaptive throttling sampling loop.");
}
catch (Exception ex)
@@ -99,2 +103,3 @@
_logger.LogError(ex, "Unexpected error in SQL adaptive throttling sampling loop.");
throw;
}
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
mikaelweave
mikaelweave previously approved these changes Jun 19, 2025
View reviewed changes
Copy link
Contributor

@mikaelweave mikaelweave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!!

@rbans96
Copy link
Contributor Author

rbans96 commented Jun 19, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

SergeyGaluzo
SergeyGaluzo reviewed Jun 20, 2025
View reviewed changes
src/Microsoft.Health.Fhir.SqlServer/Features/Storage/SqlStoreClient.cs Outdated
}

/// <summary>
/// Periodically samples the recent success and rejection rates of SQL merge resource transactions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently checks are happening on each call to merge stored procedure. This logic performs. Why is it not that simple as to return 429 on every failed for correct reason call?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyGaluzo SergeyGaluzo SergeyGaluzo approved these changes

@mikaelweave mikaelweave mikaelweave left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Enhancement Enhancement on existing functionality.
Projects
None yet
Milestone
CY25Q2/2Wk06
Development

Successfully merging this pull request may close these issues.
3 participants
@rbans96 @mikaelweave @SergeyGaluzo