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

Allow infinite TTL (time-to-live) on KeyValueCache's set. #3623

Merged
merged 9 commits into from
Dec 27, 2019

Conversation

abernix
Copy link
Member

@abernix abernix commented Dec 19, 2019

Previously, while it was possible to set very high TTL values on individual sets, it was not possible to say "Do not assign a TTL value.". This adds that possibility.

Furthermore, this allows the persisted query cache configuration (and its options PersistedQueryOptions) in Apollo Server to specify a setting and, when specified, to pass that option into the set used in ApolloServer.

Of course, that still necessitates passing a default that's isolated to Apollo Server itself, rather than a default that's able to be specified when constructing the cache.
There's another approach to solving this problem which might be worth (still? in partial replacement of?) allowing, but I couldn't convince myself of the approach.

The alternate pattern would allow the instantiation of the cache itself to override its previously hard-coded defaultSetOptions (for example), but I found that approach unfortunately complicated by varying constructor signatures on those classes which implement KeyValueCache.

In particular, they didn't seem to share a common pattern which had a home for such options that didn't necessitate the new option object becoming the second or third argument on those constructors. For example:

RedisClusterCache's

constructor(nodes: ClusterNode[], options?: ClusterOptions) {
this.client = new Redis.Cluster(nodes, options);

RedisCache's

constructor(options?: RedisOptions) {

MemcachedCache's

constructor(serverLocation: Memcached.Location, options?: Memcached.options) {

Please review commit-by-commit messages for further details.

Previously, these were all respecifying their own interface for the
options, which are the options which are passed to the `set` method when
storing an entry in those caches.  These are shared interfaces, and they all
only allow a `ttl?: number`.
Most cache stores, including Redis, support the notion of not applying a TTL
specifier on a cache record, thus allowing it to survive as long as
possible.

Of course, other factors may cause the record to be evicted (e.g. out of
memory on that node, need to make room for objects which are hotter, etc.),
those decisions are ultimately left up to the controller of those caches.

This paves the way for the next commits.
…)`s.

This will allow us to determine which have been set from tests which need to
assert those properties.
This was already the case prior to 875944e,
however the rejiggering in that PR removed that functionality, causing any
other properties present on the cache configuration to be dropped.

The original implementation can be seen here:

875944ea#diff-3467ccc78fbd796513acd747ff2110a5L212-L216

This was frustrating to sort out!
…null`.

Rather than defaulting to a behavior of not applying a TTL unless desired, all
of the current cache implementations specify a default TTL of 300.

This change introduces a specific notion of an intent to not apply a TTL
(and instead leave the eviction decision to the cache controller) by
introducing a concept of `null` for the TTL which will allow individual
subclasses of `KeyValueCache` to react accordingly.  For example, in
Memcached, this requires setting the TTL to 0, but in Redis, it requires
omitting arguments entirely.

This preserves backwards compatible behavior for existing cache
configurations by levering this new `null`ability.
Previously, there was no way to specify this value using any of our
`KeyValueCache` implementations - whether Memcached, Redis, or otherwise.

This provides that ability by setting a `ttlSeconds` to a numeric or `null`
(to specify no TTL).
* be determined by the cache's eviction policy, but the record will never
* simply expire.
*/
ttlSeconds?: number | null;
Copy link
Member Author

Choose a reason for hiding this comment

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

@trevor-scheer Can you try to convince me to change this parameter to be ttl, rather than ttlSeconds?

Considerations:

  • Against: It's come up a number of times before that parameters that define units of time could be more clear by specifying their precision as part of the name. (This is the only reason I actually put Seconds in the name.)
  • For: The existing interfaces which this translate to are still ttl, for better or for worse, but that's what they are.
  • For: It's not "seconds" when set to null, so ttlSeconds: null is a bit peculiar.
    • Along the same lines, this PR changes those existing interfaces to also support null.
  • For: While including the precision makes sense in many cases, TTLs are very often (e.g. Memcached, Redis, HTTP Cache-control, and so on). On the other hand, lru-cache sets its maxAge in milliseconds. To be honest, I find such flexibility unnecessary. Is it truly wise to cache things with sub-second precision?
  • For: Thanks to the TSDoc annotation provided directly above this, users with editors that support it are automatic given IntelliSense clarity that this in seconds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, anyone should feel free to chime in on this naming puzzle.

While I do appreciate the clarity of the unit being included in the
parameter, I noted some thoughts here:

#3623 (comment)

As an additional consideration for using `ttl` rather than `ttlSeconds`, one
could imagine an iteration on this parameter's flexibility which could support
string-based (e.g. ISO 8601, human-readable à la moment) values which would
themselves conflict (in terms of compatibility without a parameter rename)
with having a unit baked into the name.
@abernix abernix merged commit 562b0bf into master Dec 27, 2019
@abernix abernix deleted the abernix/apq-ttl-setting branch December 27, 2019 11:57
abernix added a commit that referenced this pull request Jan 24, 2020
Specifically, this documents the behavior that was introduced by the
below-referenced PR.

Ref: #3623
abernix added a commit that referenced this pull request Feb 6, 2020
* Adjust heading levels, prior to additional changes.

This makes room in the hierarchy for the work I'm about to add.

* Adjust indentation of code-fences to be within the appropriate bullets.

Otherwise, it was difficult to see the actual bullet-points.

* Lower configuration to below the fold, letting the default behavior shine.

We don't really demand configuration for this feature, which is enabled by
default.  Still, we have a lot more configuration that needs to be noted,
which can't be done without sacrificing some of the structure of this page
by inflating it with various implementation details that should be
documented, but aren't - YET! (Stand-by for next commit!)

* Add details about Redis, Redis Sentinel and Redis Cluster.

Since the `cache` options themselves are usable on other interfaces within
Apollo Server, these interfaces likely deserve to be documented on another
page eventually, but for now, this works okay and is better than them not
being documented at all.

* Add details about adjusting the cache time-to-live (TTL).

Specifically, this documents the behavior that was introduced by the
below-referenced PR.

Ref: #3623

* Provide instructions for how to disable persisted queries, if desired.

This wasn't mentioned before, but seems worth documenting!

* Edits to APQ edits (#3729)

* Edits to APQ doc improvements

* Update docs/source/performance/apq.md

* Update docs/source/performance/apq.md

Co-authored-by: Jesse Rosenberger <[email protected]>

Co-authored-by: Stephen Barlow <[email protected]>
abernix added a commit that referenced this pull request Feb 14, 2020
…ntext`.

The `persistedQueries` attribute on the GraphQLServiceContext was originally
used by the operation registry, which shared the cache with it.  This is no
longer the case.

However, while we are continuing to expand the support of the interface for
`persistedQueries`, e.g. support for default TTL values on APQ cache in
#3623, we don't want to
continually expand the API surface of what we expose to the plugin API as
we're intending to deprecate its exposure in the future.

In this particular case, it certainly doesn't need to get the `ttl` default
values which are intended for APQ only.
abernix added a commit that referenced this pull request Feb 14, 2020
The `cache` value is not required to be passed inside of `persistedQueries`
(the top-level object that's passed to Apollo Server's constructor options
to configure the APQ cache).  When it's omitted, it defaults to the (global)
`cache` also passed at the very top-level of the same constructor options.

This wasn't usually a problem in the past, but now that there are other
adjacent properties (like the `ttl` property introduced to control the
default TTL for APQ; #3623)
this has made it desirable to drop off the `cache` entirely and only pass
`ttl`.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants