Inherit default values for EntityCacheOption.Enabled and EntityCacheOption.Level from RuntimeCacheOptions#3155
Conversation
aaronburtle
requested review from
Alekhya-Polavarapu,
Aniruddh25,
JerryNixon,
RubenCerna2079,
anushakolan,
rusamant,
sourabh1007,
souvikghosh04,
stuartpa and
vadeveka
as code owners
There was a problem hiding this comment.
Pull request overview
This pull request implements inheritance of default values for entity-level cache configuration from runtime-level cache configuration. When entities don't explicitly configure cache.enabled or cache.level, they now inherit these values from the global runtime cache settings, addressing issue #3149.
Changes:
- Entity cache enabled state inherits from runtime cache enabled state when not explicitly configured
- Entity cache level inherits from runtime cache Level2 configuration (L1L2 if Level2 enabled, otherwise L1) when not explicitly configured
- Removed exceptions from
GetEntityCacheEntryTtlandGetEntityCacheEntryLevelmethods when caching is disabled, as these methods can now be safely called to retrieve inherited defaults
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Service.Tests/Caching/CachingConfigProcessingTests.cs | Added comprehensive test coverage for entity cache inheritance behavior from runtime cache settings |
| src/Core/Resolvers/SqlQueryEngine.cs | Updated to use new IsEntityCachingEnabled() method instead of entity property for inheritance-aware caching checks |
| src/Config/ObjectModel/RuntimeConfig.cs | Added IsEntityCachingEnabled() and GlobalCacheEntryLevel() methods; updated GetEntityCacheEntryTtl/Level() to support inheritance |
| src/Config/ObjectModel/RuntimeCacheOptions.cs | Added InferredLevel property to compute cache level from Level2 configuration |
| src/Config/ObjectModel/EntityCacheOptions.cs | Added UserProvidedEnabledOptions flag and updated constructor to track explicit vs inherited enabled state |
JerryNixon
left a comment
JerryNixon
left a comment
There was a problem hiding this comment.
Same as the other PR: If the old Entity.IsCachingEnabled incorporated additional logic (for example: entity absent cache block, entity defaults, or other gating like unsupported entity types), this change could enable caching where it previously wouldn’t, or vice-versa.
RubenCerna2079
left a comment
RubenCerna2079
left a comment
There was a problem hiding this comment.
LGTM! Just need to resolve comments by copilot.
|
/azp run |
|
Azure Pipelines successfully started running 6 pipeline(s). |
| [JsonConstructor] | ||
| public EntityCacheOptions(bool? Enabled = null, int? TtlSeconds = null, EntityCacheLevel? Level = null) | ||
| { | ||
| // TODO: shouldn't we apply the same "UserProvidedXyz" logic to Enabled, too? |
There was a problem hiding this comment.
Dont we need the "UserProvidedxyz" logic to Enabled?
| { | ||
| return Runtime?.Cache is not null | ||
| ? Runtime.Cache.InferredLevel | ||
| : EntityCacheOptions.DEFAULT_LEVEL; |
There was a problem hiding this comment.
The Default cache level when level 2 Cache section is NOT specified in runtime is L1 (as is evident from the InferredCacheLevel). But for EntityCacheOptions.DEFAULT_LEVEL it seems to be L1L2. Is that accurate and what we want?
Why make this change?
Closes #3149
What is this change?
Instead of having static default values for the
EntityCacheOptions.EnabledandEntityCacheOptions.Level, we inherit these default values from theRuntimeCacheOptions.How was this tested?
We add a test that validates the new behavior in
CachingConfigProcessingTestsSample Request(s)