Skip to content
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.

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

Added boundaries to re-index parameters #2103

Merged

Conversation

apurvabhaleMS
Copy link
Contributor

@apurvabhaleMS apurvabhaleMS commented Jul 20, 2021

Description

Added boundaries to ReIndex parameters
Added Unit tests
Currently, we have below ranges for different parameters based on prior experience. ( discussion with Jared )
MaximumConcurrency: 10
QueryDelayIntervalInMilliseconds: 5 - 500000
MaximumNumberOfResourcesPerQuery: 1 - 5000
TargetDataStoreUsagePercentage: 0 - 100

Related issues

Addresses [#81246].

Testing

Unite Test

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 50 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, Dependencies, Enhancement, or New-Feature
  • Tag the PR with Azure API for FHIR if this will release to the managed service
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@apurvabhaleMS apurvabhaleMS requested a review from a team as a code owner July 20, 2021 23:42
@apurvabhaleMS apurvabhaleMS added this to the S67 milestone Jul 20, 2021
@@ -220,6 +220,11 @@ public override void OnActionExecuted(ActionExecutedContext context)
}
}
}
else if (context.Exception is ArgumentOutOfRangeException argumentOutOfRangeException)
Copy link
Contributor

Choose a reason for hiding this comment

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

ArgumentOutOfRangeException

This is dangerous.
It creates blank policy to treat any ArgumentOutOfRangeException which is base .net exception as Index exception.

In my opinoin we should throw BadRequestException in case of bad parameters and we already know how to work with it in this filter.

Copy link
Contributor

Choose a reason for hiding this comment

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

BadRequestException makes sense. Another possible option is a new Exception type which extends ArgumentOutOfRange.


set
{
if (value < 1 || value > 5000)
Copy link
Contributor

Choose a reason for hiding this comment

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

5000

Where this number coming from?


set
{
if (value > 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

10

Why 10?

@apurvabhaleMS apurvabhaleMS merged commit 2e7b850 into main Jul 21, 2021
@apurvabhaleMS apurvabhaleMS deleted the personal/apurvabhale/boundaries-on-reindex-parameters branch July 21, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants