Search Param UrlLookup and TypeLookup mismatch fix#5386
Search Param UrlLookup and TypeLookup mismatch fix#5386jestradaMS wants to merge 10 commits intomainfrom
Conversation
… statuses across UrlLookup and TypeLookup
src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs
Fixed
Show fixed
Hide fixed
…to streamline search parameter status updates
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -357,30 +353,45 @@ private static HashSet<SearchParameterInfo> BuildSearchParameterDefinition( | |||
| var searchParameterDictionary = new ConcurrentDictionary<string, ConcurrentQueue<SearchParameterInfo>>(); | |||
| foreach (SearchParameterInfo searchParam in results) | |||
There was a problem hiding this comment.
Consider changing searchParam name here instead of changing it in many places below
| // URL http://hl7.org/fhir/SearchParameter/Resource-type with type Special, | ||
| // while ResourceTypeSearchParameter uses the same URL with type Token. | ||
| // Choosing the wrong type causes parser failures for _type queries. | ||
| SearchParameterInfo canonicalParam = searchParam; |
There was a problem hiding this comment.
Consider using name that closer identifies what search param origin is. Like searchParamFromUriLookup
| // while ResourceTypeSearchParameter uses the same URL with type Token. | ||
| // Choosing the wrong type causes parser failures for _type queries. | ||
| SearchParameterInfo canonicalParam = searchParam; | ||
| if (searchParam.Url != null && |
There was a problem hiding this comment.
Is searchParam valid at this point? If so, why do we check that Uri is not null?
|
How about changing our lookup data structures such that it is impossible to get different instances of SearchParameterInfo (SPI) by design? |
|
This PR solves atomicity of cache writes for UriLookup. We still don't maintain other 2 data structures (TypeLookup and hash lookup) atomically with UriLookup. It would be interesting to know the reasons for cache components to be out of sync. Is it racing writes or incorrect write workflow?. Please keep in mind that we most likely should remove racing writes and will keep all updates single directional and single threaded (from the database to cache) to have identical logic across all VMs. |
| } | ||
|
|
||
| await Task.Delay(1000); | ||
| await Task.Delay(5000); |
There was a problem hiding this comment.
Why is it bad to check every second?
| $additionalProperties["SqlServer__DeleteAllDataOnStartup"] = "false" | ||
| $additionalProperties["SqlServer__AllowDatabaseCreation"] = "true" | ||
| $additionalProperties["CosmosDb__InitialDatabaseThroughput"] = 1500 | ||
| # Cosmos DB autoscale is configured in the ARM template (10,000 RU max) |
There was a problem hiding this comment.
Does this mean we effectively increased power 6x or 1500 setting did not take effect previously?
| "azureContainerRegistryName": "[concat(substring(replace(variables('serviceName'), '-', ''), 0, min(11, length(replace(variables('serviceName'), '-', '')))), uniquestring(resourceGroup().id, variables('serviceName')))]", | ||
| "isSqlHyperscaleTier": "[equals(parameters('sqlDatabaseComputeTier'),'Hyperscale')]", | ||
| "sqlSkuCapacity": "[if(variables('isSqlHyperscaleTier'), 2, 200)]", | ||
| "sqlSkuCapacity": "[if(variables('isSqlHyperscaleTier'), 2, 800)]", |
There was a problem hiding this comment.
This looks like 4x increase in compute power. How was the lack of power manifested?
Description
This pull request introduces several important improvements and bug fixes, primarily focused on search parameter consistency and configuration updates for database throughput and autoscaling. The most significant changes ensure that the same
SearchParameterInfoinstance is used throughout the codebase, preventing subtle bugs due to race conditions. Additionally, the pull request updates database autoscaling settings and increases throughput configurations for Cosmos DB and SQL, as well as adjusts test settings for better reliability.Search parameter consistency and bug fixes:
SearchParameterInfoobjects by switching toConcurrentDictionaryand usingGetOrAdd, preventing race conditions and guaranteeing that UrlLookup and TypeLookup always reference the same instance. [1] [2] [3]BuildSearchParameterDefinitionmethod and its recursive calls to use the shareduriDictionary, ensuring object instance consistency across search parameter lookups. [1] [2] [3] [4] [5]Database configuration and autoscaling improvements:
maxThroughputto 10,000.Test reliability improvements:
Related issues
Addresses AB#183574
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)