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

Search configuration cache #1625

Closed
lmsurpre opened this issue Oct 26, 2020 · 3 comments
Closed

Search configuration cache #1625

lmsurpre opened this issue Oct 26, 2020 · 3 comments
Assignees
Labels
enhancement New feature or request performance performance search

Comments

@lmsurpre
Copy link
Member

Is your feature request related to a problem? Please describe.
SearchUtil.getFilteredBuiltinSearchParameters() reads in tenant-specific configuration and uses that config to filter the built-in search parameters down to a list of enabled search parameters on a per-tenant basis.

The fhir-config module already caches fhir-server-config.json and so reading the config should be relatively fast (no file read if the file hasn't changed), but we're still doing some extra work with each request.

Namely, if the config hasn't changed then there should be no need to recompute the set of search parameters.

Describe the solution you'd like
During server startup, we could read the fhir-server-config.json for all the tenants and pre-construct a ParametersMap for each one.

Then, we should only ever rebuild the ParametersMap if/when the fhir-server-config.json is updated for this tenant (and only if the dynamic behavior is required).

Describe alternatives you've considered
Instead of server startup, we could build the tenant-specific ParametersMap on the first request from the tenant.

Additional context
This change is highly related to #1596 because, if we get the "built-in" search parameters from the registry, then the caching here would be complicated by the fact that the registry could return different SearchParameters over time (at least when serverRegistryResourceProviderEnabled is set to true).

@punktilious
Copy link
Collaborator

Try to avoid concurrency issues, particularly with locking. Probably worth considering a ThreadLocal cache implementation for this stuff as long as the cached content isn't big (which seems to be the case here).

@lmsurpre lmsurpre self-assigned this Nov 4, 2021
@lmsurpre lmsurpre added this to the Sprint 2021-15 milestone Nov 5, 2021
lmsurpre added a commit that referenced this issue Nov 5, 2021
This is a big, end-user-visible change to the way we load search
parameters and apply filtering from fhir-server-config.

Previously, we loaded the built-in search parameters from the registry,
but then applied tenant-specific extension-search-parameters on top of
that.

Now, we have a new ExtensionSearchParametersResourceProvider that reads
the same tenant-specific extension-search-parameters (for backwards
compatibility) and provides those resources to the registry in a
tenant-aware manner.  This is #1596.

Additionally, we used to apply search filtering on each and every
request. Now, the ParametersUtil will loop through all configured
tenants and load their search parameters from the registry during
initialization. This tenant-aware search parameter map will then be
consulted anytime we need a search parameter or set of search parameters
in the context of a particular request (for a particular tenant).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Nov 5, 2021
This is a big, end-user-visible change to the way we load search
parameters and apply filtering from fhir-server-config.

Previously, we loaded the built-in search parameters from the registry,
but then applied tenant-specific extension-search-parameters on top of
that.

Now, we have a new ExtensionSearchParametersResourceProvider that reads
the same tenant-specific extension-search-parameters (for backwards
compatibility) and provides those resources to the registry in a
tenant-aware manner.  This is #1596.

Additionally, we used to apply search filtering on each and every
request. Now, the ParametersUtil will loop through all configured
tenants and load their search parameters from the registry during
initialization. This tenant-aware search parameter map will then be
consulted anytime we need a search parameter or set of search parameters
in the context of a particular request (for a particular tenant).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Nov 5, 2021
This is a big, end-user-visible change to the way we load search
parameters and apply filtering from fhir-server-config.

Previously, we loaded the built-in search parameters from the registry,
but then applied tenant-specific extension-search-parameters on top of
that.

Now, we have a new ExtensionSearchParametersResourceProvider that reads
the same tenant-specific extension-search-parameters (for backwards
compatibility) and provides those resources to the registry in a
tenant-aware manner.  This is #1596.

Additionally, we used to apply search filtering on each and every
request. Now, the ParametersUtil will loop through all configured
tenants and load their search parameters from the registry during
initialization. This tenant-aware search parameter map will then be
consulted anytime we need a search parameter or set of search parameters
in the context of a particular request (for a particular tenant).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Nov 5, 2021
This is a big, end-user-visible change to the way we load search
parameters and apply filtering from fhir-server-config.

Previously, we loaded the built-in search parameters from the registry,
but then applied tenant-specific extension-search-parameters on top of
that.

Now, we have a new ExtensionSearchParametersResourceProvider that reads
the same tenant-specific extension-search-parameters (for backwards
compatibility) and provides those resources to the registry in a
tenant-aware manner.  This is #1596.

Additionally, we used to apply search filtering on each and every
request. Now, the ParametersUtil will loop through all configured
tenants and load their search parameters from the registry during
initialization. This tenant-aware search parameter map will then be
consulted anytime we need a search parameter or set of search parameters
in the context of a particular request (for a particular tenant).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Nov 9, 2021
* issues #1625 and #1596 - apply search parameter filters on init

This is a big, end-user-visible change to the way we load search
parameters and apply filtering from fhir-server-config.

Previously, we loaded the built-in search parameters from the registry,
but then applied tenant-specific extension-search-parameters on top of
that.

Now, we have a new ExtensionSearchParametersResourceProvider that reads
the same tenant-specific extension-search-parameters (for backwards
compatibility) and provides those resources to the registry in a
tenant-aware manner.  This is #1596.

Additionally, we used to apply search filtering on each and every
request. Now, the ParametersUtil will loop through all configured
tenants and load their search parameters from the registry during
initialization. This tenant-aware search parameter map will then be
consulted anytime we need a search parameter or set of search parameters
in the context of a particular request (for a particular tenant).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* JDBC test cleanup

LocationParmBehaviorUtilTest wasn't restoring the FHIRRequestContext
after running.
Additionally, I removed the 'static' modifier from the before and after
methods...its not needed for TestNG and, in fact, TestNG warns against
using static methods for this.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Handle missing config more gracefully

The CQL SearchParameterResolverTest was calling
SearchUtil.getSearchParameters without any fhir-server-config files in
the home directory.
This was causing a NullPointerException while initializing
ParametersUtil.
The updated logic will now load all parameters in such cases...just as
if fhir-server-config was there but without a `resources` PropertyGroup.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* updated documentation to reflect new behavior

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* General cleanup

Extracted private helpers from ParametersUtil.computeTenantSPs and added
some comments.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Update SearchUtil.getSearchParameters method sig

This one now returns a map from code to SearchParameter instead of just
a the list of SearchParameter. This way, we can support cases where the
explicitly-configured code differs from the code in the SearchParameter.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Update ParametersMap to store disambiguated results

Previously, ParametersMap had to store all the built-in parameters and
make those available to SearchUtil for filtering/disambiguation.

Now that we apply filtering before populating ParametersMap, we can
select a "winner" in cases where there are multiple parameter with the
same code (or multiple parameters with the same url / canonical).

For now, I chose a "last insert wins" approach...which means that the
order we load the map in will matter.

Note: this change uncovered an issue in our fhir-persistence search
tests where we defined a parameter with code "code" that conflicts with
a base spec parameter. The fix was to disambiguate via
fhir-server-config.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* minor updates per review feedback

mostly copyright dates...

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
Copy link
Member Author

lmsurpre commented Nov 9, 2021

The "cache" was implemented as a revision to the existing ParameterUtils class. Now, upon initialization, that will loop over all configured tenants (i.e. all directories under the fhir-server's config dir) and apply the search filtering rules therein to the search parameters loaded from the registry.

@prb112
Copy link
Contributor

prb112 commented Nov 12, 2021

QA Complete

Looks good

Test 1. Server Startup with a single Parameter

[11/12/21, 15:52:14:076 UTC] 00000022 FHIRConfigura 1 Returning list of tenant ids: [default]
[11/12/21, 15:52:14:076 UTC] 00000022 ParametersUti 1 Computing search parameters for tenant default
[11/12/21, 15:52:14:077 UTC] 00000022 TenantSpecifi 1 Loading config file from /wlp/usr/servers/fhir-server/config/default/extension-search-parameters.json
[11/12/21, 15:52:14:077 UTC] 00000022 TenantSpecifi 1 The file loaded is [file:/wlp/usr/servers/fhir-server/config/default/extension-search-parameters.json]
[11/12/21, 15:52:14:081 UTC] 00000022 TenantSpecifi 1 Loaded SearchParameters for tenant-id 'default' and added it to the cache.
[11/12/21, 15:52:15:778 UTC] 00000022 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/Resource-content|4.0.1' with missing expression
[11/12/21, 15:52:15:778 UTC] 00000022 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/DomainResource-text|4.0.1' with missing expression
[11/12/21, 15:52:15:780 UTC] 00000022 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/Resource-query|4.0.1' with missing expression

Test 2. Added SearchParameter post startup

Waited 1 minute, No evidence of reloading
Request https://localhost:9443/fhir-server/api/v4/Patient?_format=application/json&_page=1&birthdate=le2007&birthdate=ge1964&gender=female&favorite-color=blue
Sees "Search parameter 'favorite-color' for resource type 'Patient' was not found."

Test 3. Restart and Rerun

{
"resourceType": "Bundle",
"id": "7cf62b7a-f296-4e02-81ad-4da8832ef683",
"type": "searchset",
"total": 0,
"link": [
{
"relation": "self",
"url": "https://localhost:9443/fhir-server/api/v4/Patient?_count=10&birthdate=le2007&birthdate=ge1964&gender=female&favorite-color=blue&_page=1"
}
]
}

Shows no error / invalid which is what is expected

Test 4. Restart and Run with tenant1 and tenant2

[11/12/21, 16:12:48:696 UTC] 00000027 FHIRConfigura 1 Returning list of tenant ids: [default, tenant1, tenant2]
[11/12/21, 16:12:48:696 UTC] 00000027 ParametersUti 1 Computing search parameters for tenant default
[11/12/21, 16:12:48:697 UTC] 00000027 TenantSpecifi 1 Loading config file from /wlp/usr/servers/fhir-server/config/default/fhir-server-config.json
[11/12/21, 16:12:48:792 UTC] 00000027 TenantSpecifi 1 Loaded PropertyGroup for tenant-id 'default' and added it to the cache.
[11/12/21, 16:12:48:795 UTC] 00000027 TenantSpecifi 1 Loading config file from /wlp/usr/servers/fhir-server/config/default/extension-search-parameters.json
[11/12/21, 16:12:48:796 UTC] 00000027 TenantSpecifi 1 The file loaded is [file:/wlp/usr/servers/fhir-server/config/default/extension-search-parameters.json]
[11/12/21, 16:12:48:802 UTC] 00000027 TenantSpecifi 1 Loaded SearchParameters for tenant-id 'default' and added it to the cache.
[11/12/21, 16:12:51:405 UTC] 00000027 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/Resource-content|4.0.1' with missing expression
[11/12/21, 16:12:51:405 UTC] 00000027 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/DomainResource-text|4.0.1' with missing expression
[11/12/21, 16:12:51:407 UTC] 00000027 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/Resource-query|4.0.1' with missing expression
[11/12/21, 16:12:51:410 UTC] 00000027 ParametersUti 1 Computing search parameters for tenant tenant1
[11/12/21, 16:12:51:410 UTC] 00000027 TenantSpecifi 1 Loading config file from /wlp/usr/servers/fhir-server/config/tenant1/fhir-server-config.json
[11/12/21, 16:12:51:411 UTC] 00000027 TenantSpecifi 1 Loaded PropertyGroup for tenant-id 'tenant1' and added it to the cache.
[11/12/21, 16:12:51:414 UTC] 00000027 TenantSpecifi 1 Loading config file from /wlp/usr/servers/fhir-server/config/tenant1/extension-search-parameters.json
[11/12/21, 16:12:51:415 UTC] 00000027 TenantSpecifi 1 The file loaded is [file:/wlp/usr/servers/fhir-server/config/tenant1/extension-search-parameters.json]
[11/12/21, 16:12:51:418 UTC] 00000027 TenantSpecifi 1 Loaded SearchParameters for tenant-id 'tenant1' and added it to the cache.
[11/12/21, 16:12:51:428 UTC] 00000027 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/DomainResource-text|4.0.1' with missing expression
[11/12/21, 16:12:51:430 UTC] 00000027 ParametersUti 1 Computing search parameters for tenant tenant2
[11/12/21, 16:12:51:430 UTC] 00000027 TenantSpecifi 1 Loading config file from /wlp/usr/servers/fhir-server/config/tenant2/fhir-server-config.json
[11/12/21, 16:12:51:431 UTC] 00000027 TenantSpecifi 1 Loaded PropertyGroup for tenant-id 'tenant2' and added it to the cache.
[11/12/21, 16:12:51:431 UTC] 00000027 TenantSpecifi 1 Loading config file from /wlp/usr/servers/fhir-server/config/tenant2/extension-search-parameters.json
[11/12/21, 16:12:51:432 UTC] 00000027 TenantSpecifi 1 The file loaded is [file:/wlp/usr/servers/fhir-server/config/tenant2/extension-search-parameters.json]
[11/12/21, 16:12:51:433 UTC] 00000027 TenantSpecifi 1 Loaded SearchParameters for tenant-id 'tenant2' and added it to the cache.
[11/12/21, 16:12:51:440 UTC] 00000027 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/Resource-content|4.0.1' with missing expression
[11/12/21, 16:12:51:441 UTC] 00000027 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/DomainResource-text|4.0.1' with missing expression
[11/12/21, 16:12:51:442 UTC] 00000027 ParametersMap 1 Skipping parameter 'http://hl7.org/fhir/SearchParameter/Resource-query|4.0.1' with missing expression

The three tenants are loaded and their parameters are available.

Test 5. Startup and Check FS Events during query

PS=$(ps -ef | grep -i fhir-server | grep -v grep | awk '{print $2}')
sudo fs_usage -w ${PS} | grep -i json | grep extension-se

I did not see any FS events that showed a reloading for the search-parameters json

@prb112 prb112 closed this as completed Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request performance performance search
Projects
None yet
Development

No branches or pull requests

4 participants