Distinguish caching values and storing values persistently #606
Conversation
svix-daniel
force-pushed
the
daniel/cache-store-split
branch
2 times, most recently
from
27fabf1
to
42c45e1
Compare
svix-daniel
force-pushed
the
daniel/cache-store-split
branch
from
42c45e1
to
fac4f0d
Compare
svix-daniel
force-pushed
the
daniel/cache-store-split
branch
from
fac4f0d
to
bc17862
Compare
| # multiple API servers running, please use the redis backend or some functionality, (e.g. Idempotency) | ||
| # may fail to work correctly. | ||
| # The memory backend is recommended if you only have one instance running, if you have multple | ||
| # services running, then pleas use the redis backend or features like idempotency will fail to work |
There was a problem hiding this comment.
| # services running, then pleas use the redis backend or features like idempotency will fail to work | |
| # services running, then please use the redis backend or features like idempotency will fail to work |
|
|
||
| #[derive(Clone)] | ||
| pub struct NoCache; | ||
| pub struct NoKeyValueStore; |
There was a problem hiding this comment.
If you are already renaming it, please rename it to KeyValueStoreNone to be consistent with the rest.
| impl AppEndpointKey { | ||
| // FIXME: Rewrite doc comment when AppEndpointValue members are known | ||
| /// Returns a key for fetching all cached endpoints for a given organization and application. | ||
| pub fn new(org: OrganizationId, app: ApplicationId) -> AppEndpointKey { | ||
| AppEndpointKey(format!("{}_APP_v3_{}_{}", Self::PREFIX_CACHE, org, app)) | ||
| AppEndpointKey(format!("_APP_v3_{}_{}", org, app)) |
There was a problem hiding this comment.
This looks wrong? Shouldn't it be:
| AppEndpointKey(format!("_APP_v3_{}_{}", org, app)) | |
| AppEndpointKey(format!("{}_{}", org, app)) |
And I'm not sure where the _ between the prefix and the key go.
I gotta say, I'm not sure I'm a huge fan of having the prefix defined separately than the function, it makes it very hard to know what's going on. It's much easier to just see:
AppEndpointKey(format!("SVIX_CACHE_APP_v3_{}_{}", org, app))All in one place.
| pub first_failure_at: DateTimeUtc, | ||
| } | ||
|
|
||
| kv_def!(FailureCacheKey, FailureCacheValue, "SVIX_FAILURE_CACHE"); | ||
| store_kv_def!(FailureKey, FailureValue, "FAILURES_"); |
There was a problem hiding this comment.
Missing the Svix prefix.
After seeing this, I'm 100% convinced about my other comment of splitting the prefix being confusing.
There was a problem hiding this comment.
I made some comments, though I think the may problem is that this PR is just too big.
For example, I'd split the fact that cache can't be null (it's checked in idempotency) to its own PR, and just get that in.
I'd then have another PR that splits the cache and store to two, but everything will still use cache.
I'd then have another PR (or better yet, commit on the second PR) to start using the store.
It's just really really hard to understand what's going on and why a change is needed and what may be a mistake.
I'm also 95% certain we need to remove the prefix from the cache/store structs and just have the key fully defined in the new() function that we already have for each of them anyway.
Before these changes we had used the concept of the cache for features such as idempotency and others which have persistent data. This conflicts with the definition of caching, which is meant to work with ephemeral data. Thus these changes distinguish the concepts with a
SharedStoreand aCachestructure, which internally, both use aKvBackendimplementor.Note that this is not exposed in the configuration, but this split is needed as additional functionality unique to persistent storage or genuine caching is implemented.
Additionally this updates prior uses of the cache to utilize the structure most appropriate.