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

Add an application cache for REST API #9173

Closed
wants to merge 48 commits into from

Conversation

jascks
Copy link
Contributor

@jascks jascks commented Aug 27, 2024

Description:

Very preliminary draft to get feedback on general approach to the solution. There is still more implementation and testing to be done.

There are many times that the same request from different sessions all hit our REST server even though cloud CDN is enabled and the responses have proper cache control header set.

There are also periodic high rate of response code 304 from our REST server because some clients will issue a HEAD request with cache id to probe if the cache is still valid. Due to lack of head handler in our js REST module, it ends up to go though the get handler and query data, then expressjs learns the cache is still valid, zeros out the response body and sends a 304 response.

We should add a application layer cache to handle such scenarios so the rest server doesn't unnecessarily go to the database for the data.

Related issue(s):

Fixes #9111

Notes for reviewer:

  • Still working out Snyk code security check failure.
  • Working on getting performance testing results.
  • Determine Redis maxMemory.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@jascks jascks added enhancement Type: New feature performance rest Area: REST API labels Aug 27, 2024
@jascks jascks added this to the 0.113.0 milestone Aug 27, 2024
@jascks jascks self-assigned this Aug 27, 2024
@jascks jascks linked an issue Aug 27, 2024 that may be closed by this pull request
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 92.66055% with 8 lines in your changes missing coverage. Please review.

Project coverage is 92.57%. Comparing base (19d22d2) to head (2b65162).
Report is 86 commits behind head on main.

Files with missing lines Patch % Lines
hedera-mirror-rest/server.js 88.23% 4 Missing ⚠️
hedera-mirror-rest/cache.js 88.88% 2 Missing ⚠️
hedera-mirror-rest/middleware/metricsHandler.js 0.00% 1 Missing ⚠️
...era-mirror-rest/middleware/responseCacheHandler.js 97.87% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #9173      +/-   ##
============================================
- Coverage     92.57%   92.57%   -0.01%     
  Complexity     7047     7047              
============================================
  Files           915      917       +2     
  Lines         29870    29953      +83     
  Branches       3773     3788      +15     
============================================
+ Hits          27653    27729      +76     
- Misses         1446     1453       +7     
  Partials        771      771              

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

hedera-mirror-rest/server.js Outdated Show resolved Hide resolved
@steven-sheehy steven-sheehy modified the milestones: 0.113.0, 0.114.0 Sep 3, 2024
Jeff Schmidt added 13 commits September 4, 2024 14:49
…nstance initialization order.

Signed-off-by: Jeff Schmidt <[email protected]>
…ased on original max-age. Clean up.

Signed-off-by: Jeff Schmidt <[email protected]>
…eware will compress it. Always call next();

Signed-off-by: Jeff Schmidt <[email protected]>
Jeff Schmidt added 3 commits September 12, 2024 15:19
@jascks jascks marked this pull request as ready for review September 13, 2024 03:07
@steven-sheehy steven-sheehy modified the milestones: 0.114.0, 0.115.0 Sep 16, 2024
Copy link

sonarcloud bot commented Sep 30, 2024

@edwin-greene edwin-greene modified the milestones: 0.115.0, 0.116.0 Oct 1, 2024
@steven-sheehy steven-sheehy assigned jnels124 and unassigned jascks Oct 10, 2024
@steven-sheehy steven-sheehy modified the milestones: 0.116.0, 0.117.0 Oct 15, 2024
@jnels124 jnels124 closed this Oct 23, 2024
@steven-sheehy steven-sheehy deleted the 9111-add-an-application-cache-for-rest-api branch October 24, 2024 00:34
@steven-sheehy steven-sheehy removed this from the 0.117.0 milestone Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Type: New feature performance rest Area: REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an application cache for REST API
4 participants