Skip to content

Conversation

bartelink
Copy link
Collaborator

@bartelink bartelink commented May 18, 2023

#386 focuses on the implementation. This PR generalises surrounding functionality in preparation for this

  • fixes some stale comments re caching
  • extract a formal IReloadable interface for the cache to interact with
  • generalises the supersedes store function used to compare cache entries to a compare function that can also confirm an entry is identical (in order to be able to track when a cache entry has been successfully validated)
  • generalises the allowStale flag to be a maxStaleness: TimeSpan (true/false becomes TimeSpan.MaxValue/TimeSpan.Zero)
  • simplifies/clarifies the AsyncCacheCell internals to align with the requirements of the ReadThrough mode implementation
  • makes the CacheEntry type internal
  • adds baseline tests for the Caching behavior (in a long overdue Equinox.Core.Tests project)
  • removes dead code

interface ICategory<'event, 'state, 'context> with
member _.Load(log, categoryName, streamId, streamName, allowStale, requireLeader, ct) = task {
match! tryReadCache streamName with
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

note this was not computing a correct key in the case of SlidingWindowPrefixed (fixed in #386)

@bartelink bartelink marked this pull request as ready for review May 18, 2023 10:19
@bartelink bartelink mentioned this pull request May 18, 2023
4 tasks
interface Caching.IReloadableCategory<'event, 'state, 'context> with
member _.Load(log, _categoryName, _streamId, streamName, _allowStale, requireLeader, ct) =
interface ICategory<'event, 'state, 'context> with
member _.Load(log, _categoryName, _streamId, streamName, _maxStaleness, requireLeader, ct) =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably pragmatic, but seeing that every store implementation is ignoring the _maxStaleness parameter has me wondering if this is the right interface 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The other idea I have in that space is for the layers inward of the cache to have a LoadOrReload signature with a origin: (token*state) voption, rather than Load+Reload (should that be called Fetch ?). ICategory and ICategoryImpl ;)

@bartelink bartelink merged commit c7c4f62 into master Jun 3, 2023
@bartelink bartelink deleted the cache-cleanup branch June 3, 2023 00:06
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.

2 participants