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

In-Memory Cache #1881

Merged
merged 19 commits into from
Dec 7, 2023
Merged

In-Memory Cache #1881

merged 19 commits into from
Dec 7, 2023

Conversation

seantleonard
Copy link
Contributor

Why make this change?

What is this change?

  • Implements the design in REST and GraphQL Query Caching Design Doc #1801. The one behavior which isn't in this PR is caching GraphQL "list" queries. That will come in a separate commit once i resolve the errors that arise with generics and the DataReaderHandler delegates.
  • Creates a DabCacheService which is used in database queries (REST/GraphQL) such that requests and responses are cached in memory.
  • This PR is not dependent on Add Cache properties to runtime config file with JSON Serialize/Deserialize #1865. However, once that PR and this PR are merged, a third follow-up PR will make a small adjustment in SqlQueryEngine.ExecuteAsync(...) such that the cacheEntryTtl and cacheEnabled values are resolved from runtime config and used determine whether to use the cache and how to use the cache.

How was this tested?

  • Integration Tests -> integration tests included which mock the QueryExecutor and test how the DabCacheService handles request metadata and caching method execution. The integration tests aim to test the behavior of the DabCacheService and intentionally do not make private methods public or break out key generation and entry size calculation into separate classes. Additionally, the integration tests are not meant to test the functionality of FusionCache. That library has its own tests which validate functionality.

Sample Request(s)

Until all related PRs are merged, this feature is gated by a feature flag. To add the feature flag, add the following snippet to the AppConfig.json:

// Define feature flags in config file
    "FeatureManagement": {
        "CachingPreview": true, // Turn-On feature
    }

Point requests will be cached with a default 10 second ttl. You can try out whether response times are quicker for your use case.

Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Looks good, posting comments so far, tests remaining..

src/Service/Startup.cs Outdated Show resolved Hide resolved
src/Directory.Packages.props Outdated Show resolved Hide resolved
src/Core/Resolvers/SqlQueryEngine.cs Outdated Show resolved Hide resolved
src/Core/Services/Cache/DabCacheService.cs Show resolved Hide resolved
src/Core/Services/Cache/DabCacheService.cs Show resolved Hide resolved
src/Core/Services/Cache/DabCacheService.cs Show resolved Hide resolved
src/Core/Resolvers/SqlQueryEngine.cs Show resolved Hide resolved
Copy link
Contributor

@Aniruddh25 Aniruddh25 left a comment

Choose a reason for hiding this comment

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

Very Comprehensive tests, left some questions.

@seantleonard
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 6 pipeline(s).

Copy link
Contributor

@severussundar severussundar left a comment

Choose a reason for hiding this comment

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

Neat. Good usage of mocks for writing the integration tests.

Few nits, otherwise, LGTM!

…ache or entity cache settings). Updates appsettings json file with feature flag defaulted to false. added feature flag class with constant.
@seantleonard
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@seantleonard
Copy link
Contributor Author

/azp run

Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@seantleonard
Copy link
Contributor Author

/azp run

@seantleonard seantleonard dismissed Aniruddh25’s stale review December 7, 2023 05:11

Addressed comments and received additional reviews.

Copy link

Azure Pipelines successfully started running 6 pipeline(s).

@seantleonard seantleonard enabled auto-merge (squash) December 7, 2023 05:12
@seantleonard seantleonard merged commit 40dab94 into main Dec 7, 2023
12 checks passed
@seantleonard seantleonard deleted the dev/seantleonard/inmemorycache branch December 7, 2023 05:34
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.

5 participants