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

Implement reusing of async jobs for equivalent requests. #1414

Merged
merged 14 commits into from
Dec 1, 2023
Merged

Conversation

piotrszul
Copy link
Collaborator

Resolves #1313.

@piotrszul piotrszul self-assigned this Apr 6, 2023
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage: 90.12% and project coverage change: +0.06 🎉

Comparison is base (6f4ea20) 85.43% compared to head (bf8863a) 85.49%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1414      +/-   ##
============================================
+ Coverage     85.43%   85.49%   +0.06%     
  Complexity      135      135              
============================================
  Files           337      339       +2     
  Lines          7751     7793      +42     
  Branches        515      518       +3     
============================================
+ Hits           6622     6663      +41     
  Misses          839      839              
- Partials        290      291       +1     
Impacted Files Coverage Δ
...au/csiro/pathling/aggregate/AggregateProvider.java 100.00% <ø> (ø)
...ava/au/csiro/pathling/extract/ExtractProvider.java 94.44% <ø> (ø)
.../java/au/csiro/pathling/update/ImportProvider.java 80.00% <ø> (ø)
.../main/java/au/csiro/pathling/async/RequestTag.java 83.33% <83.33%> (ø)
...main/java/au/csiro/pathling/async/JobRegistry.java 88.23% <88.23%> (-11.77%) ⬇️
...ava/au/csiro/pathling/async/RequestTagFactory.java 89.47% <89.47%> (ø)
...main/java/au/csiro/pathling/async/AsyncAspect.java 85.71% <90.90%> (-5.96%) ⬇️
...ver/src/main/java/au/csiro/pathling/async/Job.java 88.88% <100.00%> (+13.88%) ⬆️
...main/java/au/csiro/pathling/async/JobProvider.java 66.66% <100.00%> (ø)
...a/au/csiro/pathling/config/AsyncConfiguration.java 66.66% <100.00%> (+16.66%) ⬆️
... and 1 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

piotrszul and others added 2 commits April 18, 2023 16:44
# Conflicts:
#	fhir-server/src/main/java/au/csiro/pathling/aggregate/AggregateProvider.java
#	fhir-server/src/test/java/au/csiro/pathling/test/integration/AsyncTest.java
@johngrimes johngrimes added the optimisation Improvement to the performance or efficiency of existing functionality label May 3, 2023
Copy link
Member

@johngrimes johngrimes left a comment

Choose a reason for hiding this comment

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

Thanks @piotrszul, I've added a few comments / questions.

Once these have been looked at, I think the best way to prove this out is to merge to a release branch and deploy the pre-release image with a conventional cache setup. We will need to do this for both unauthenticated and authenticated configurations.

I have created a couple of related issues that will need to be resolved before this change can be deployed:

  • aehrc/pathling-helm#2
  • aehrc/pathling-deployment#6

Base automatically changed from release/6.2.0 to main May 23, 2023 03:29
…udedFromCacheKey` and added documentation for it.

Removed principal from the asyn request cache key it duplicated the Authorization header (for Jwt).
@johngrimes
Copy link
Member

I've moved the Helm chart into the main repository, and I've decided to defer the update of the configuration management for now.

I've merged this into the 6.4.0 release.

@johngrimes johngrimes merged commit 5c04c12 into main Dec 1, 2023
@johngrimes johngrimes deleted the issue/1313 branch December 1, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimisation Improvement to the performance or efficiency of existing functionality
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Improve cacheability of async requests
2 participants