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

Updates to the canonical validation resource cache in VersionSpecificWorkerContextWrapper #6506

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

codeforgreen
Copy link
Collaborator

@codeforgreen codeforgreen commented Nov 23, 2024

Fixes #6505

Please note that the fix will be backported to version 7.4 as well.

What was done:

  • main fix to increase the cache expiry from 10s to 10min. This is the same as the underlying persistence cache (see CacheTimeouts).
  • some internal refactoring in VersionSpecificWorkerContextWrapper such that all methods that retrieve validation resources go through the cache using same method. CodeSystem resources in particular was not using this cache and it now is using it.
  • updated all implementors of IValidationSupport to use Logs.getTerminologyTroubleshootingLog()
  • added a bunch of new tests on VersionSpecificWorkerContextWrapper related to the cache and also testing the methods to fetch validation resources at different levels (unit and integration).

While working on this issue, some opportunity for optimization was observed which will be tackled in a different issue. Seems that the package installer does not generate snapshots properly. Some snapshots are not saved to the database and since the internal cache does not store generated snapshots when it encounters such cases, that results in some validate calls to be slow because they generate the missing requires snapshots every time the cache entry for the profile expires.

… that canonical CodeSystem resources are also cached. Use the terminology troubleshooting logs throughout the validation module.
Copy link

Formatting check succeeded!

Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.46%. Comparing base (406db33) to head (2091291).
Report is 100 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #6506      +/-   ##
============================================
- Coverage     83.54%   83.46%   -0.09%     
- Complexity    27432    27882     +450     
============================================
  Files          1707     1752      +45     
  Lines        106185   108083    +1898     
  Branches      13397    13563     +166     
============================================
+ Hits          88710    90208    +1498     
- Misses        11750    12045     +295     
- Partials       5725     5830     +105     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jamesagnew
Copy link
Collaborator

FYI @codeforgreen I believe this PR should be redundant once this merges (it also fixes the same issue by just never expiring StructureDefinions): #6508

would be returned which suggest the profile was not applied properly or used.
That happens because the validation cache entry expired within the $validate call
and the FHIR Core Validation library expects the same object instance for the same profile to be returned.
This has been fixed by increasing the cache expiry to 10min."
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Does this mean we are still vulnerable to rare failures if a cache entry expires in the middle of processing?

ourLog.warn(
String.format("Multiple results found for URL '%s', only the first will be considered.", url));
if (results != null && !results.isEmpty()) {
if (results.size() > 1 && ourLog.isWarnEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

trivial: the ourLog.isWarnEnabled() check seems redundant since the call to warn uses a format string.


import java.io.IOException;

public class DefaultProfileValidationSupportNpmStrategy extends NpmPackageValidationSupport {
private static final Logger ourLog = LoggerFactory.getLogger(DefaultProfileValidationSupportNpmStrategy.class);
private static final Logger ourLog = Logs.getTerminologyTroubleshootingLog();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice to see a new troubleshooting log.

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.

Unexpected validation errors occur when the profile resource cache entry expires within a $validate call
3 participants