Add support for cache tag invalidation #1169

clarencenpy · 2018-06-13T12:36:20Z

TODO:

Update CHANGELOG.md with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

See http://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-9.html

clarencenpy · 2018-06-13T12:43:29Z

@martijnwalraven I've implemented basic support for Memcached/Redis for the KeyValueCache interface, which are currently just simple wrappers over other clients. Supports get/set with ttl.
I've written some tests against external databases just to make sure that things work for now, but they will probably be unnecessary going forward (we just need to test the cache-tag invalidation logic).

martijnwalraven

This is a great start! Really happy with the code quality and attention to testing. It seems we should be able to get a basic version in sooner than expected!

martijnwalraven · 2018-06-13T13:41:42Z

packages/apollo-server-caching/package.json

    "jest": "^23.1.0",
+    "jest-each": "^23.1.0",


I didn't know about jest-each, but that's a neat solution! it seems support for this has (very) recently become part of core Jest though, so maybe we don't even need the dependency (see README).

great catch!

martijnwalraven · 2018-06-13T13:50:02Z

packages/apollo-server-caching/src/__mocks__/common.ts

@@ -0,0 +1,6 @@
+export const servers = {


Having to deal with external dependencies means these are not really unit tests and we probably don't want to run them as part of CI for example. I think they are great as integration tests, so I'd like to keep them (especially if we add cache tags on top of the basic features), but I'm not sure how we should organize them to separate them from the unit tests and what the best practices around integration testing are (@jbaxleyiii may have some ideas there).

A script to start a Memcached and Redis server ready for these tests would be useful I think. And maybe we could run it as part of a npm run test:integration command.

I'm a lot less familiar with Redis (though I suspect it should be similarly easy) but Memcached should be incredibly easy to mock. In fact, a quick search turned up memcached-mock and redis-mock.

If it's at all feasible to use those, rather than implementing a script to start one (which would need to struggle with cross-platform network/process implementation details, not to mention installation), that seems preferable.

agreed! these integration tests probably don't need to be run with CI, its just for me to validate that the dependencies on external libraries work as expected. I'm running the databases with docker, will try to set it up as a npm script!

@abernix that's awesome! I will use proxyquire to mock memcached and node-redis for testing (:

update: turns out I cant get proxyquire to work with typescript. Using jest.mock instead and it works great!

Even better :)

martijnwalraven · 2018-06-13T13:50:37Z

packages/apollo-server-caching/src/__tests__/connectors.test.ts

+  it('is able to expire keys based on ttl', async () => {
+    await keyValueCache.set('short', 's', { ttl: 1 });
+    await keyValueCache.set('long', 'l', { ttl: 5 });
+    await delay(1500);


Having to depend on actual time passing (as opposed to mocking timers) is another reason this isn't really suitable as a unit test.

Is it acceptable to leave out tests for key expiry?

@clarencenpy This may blast the complexity of the tests, but I can recommend lolex for doing time stuff. You can see an example here: https://github.com/n1ru4l/react-time-ago/blob/29006c7a2080626ec4206560fe4ecf69c3ca079b/src/index.test.js#L52

For sure! I will test it out with the mocked databases and it will be great if that plays well together! Thanks 👍

Using @martijnwalraven 's mockDate() (for calls to global date) and jest.useFakeTimers() (for calls to setTimeout). Seems to work great with the mock libs.

Great! Yeah, as long as we're using mocks for the databases we don't need actual delays.

martijnwalraven · 2018-06-13T13:52:05Z

packages/apollo-server-caching/src/connectors/memcached.ts

+  private client;
+  private defaultSetOptions = {
+    ttl: 300,
+    tags: [],


I'm not sure if default tags make sense. But a default ttl seems really useful, so maybe we could even expose that as a public property / configuration option.

martijnwalraven · 2018-06-13T13:53:55Z

packages/apollo-server-caching/src/connectors/memcached.ts

+
+  close(): Promise<void> {
+    this.client.end();
+    return Promise.resolve();


If you make the function async I think you may not need the explicit Promise.resolve() here.

(Assuming client.end() doesn't take a callback like the Redis equivalent.)

evans

Great stuff, just a few comments about async function and one for the test and then we can merge!

evans · 2018-06-14T21:04:33Z

packages/apollo-server-caching/src/connectors/memcached.ts

+    }
+  }
+
+  async flush(): Promise<void> {


With an async function, thrown errors are automatically wrapped in a promise rejection and returned values are wrapped in a promise resolution, so there's no need to wrap the this.client.flush call.

I see! This simplifies the code significantly!

evans · 2018-06-14T21:20:54Z

packages/apollo-server-caching/src/__tests__/connectors.test.ts

+
+  it('is able to expire keys based on ttl', async () => {
+    await keyValueCache.set('short', 's', { ttl: 1 });
+    await keyValueCache.set('long', 'l', { ttl: 5 });


Let's make sure we can read these values right after they have been set and then ensure that both are invalidated after advancing the time past 5 seconds

…ollo/apollo-server with conflicts.

clarencenpy · 2018-06-15T18:58:54Z

packages/apollo-datasource-rest/src/connectors/memcached.ts

+      t: tags,
+    };
+
+    await this.client.set(key, JSON.stringify(payload), ttl);


Instead of serializing tag metadata together with the key, we could also store metadata in separate namespaced keys:
set('key1', 'value1', ['tag1', 'tag2']) gets stored like this:

key1: value1 meta-v-key1: <version number> meta-t-key1: 'tag1||tag2'

martijnwalraven · 2018-06-15T20:39:15Z

packages/apollo-datasource-rest/src/connectors/memcached.ts

+    // augment data with tags and version
+    const version = ++this.logicalClock;
+    const payload = {
+      v: version,


Hmmm, this wasn't really what I had in mind, and I don't think this will work as is. If we keep a logical clock in memory, that will be per process. So it seems there is no way to keep this working correctly across multiple server instances talking to the same store.

My idea was that we would store <tag>: <version> pairs in cache entries, and also keep separate <tag>: <version> entries. We could then use store primitives to safely increment the version for a tag entry without the need for a global clock.

I was thinking that it would be easy to move the logical clock into the key-value store as an entry of its own, once we think about multiple server processes. It seems to me like an easier thing to keep track of? Refer to this commit

I think this boils down to a tradeoff between managing versions for every tag (more complex code, and reading all tag versions for each set operation), vs managing a global version number (will we pay in performance?). Would be happy to learn about other alternatives that I am missing!

Hmmm, interesting! I'm worried keeping a global version instead of a version per tag will become a bottleneck and/or source of concurrency issues, but other people may have a better idea of the impact. I suspect that will depend on the characteristics of the store.

On Memcache, one thing to take into consideration is that the global version key will only be stored on a single node, so that will receive a lot of requests. And if it goes down, you basically lose the ability to validate all data (although the same is true for a subset of the data for per-tag version keys).

martijnwalraven · 2018-06-15T20:45:03Z

types/fetch/index.d.ts

@@ -0,0 +1,111 @@
+declare function fetch(


We shouldn't need declarations for fetch and related types here, we'll get that from apollo-server-env.

martijnwalraven · 2018-06-15T20:45:52Z

types/url/index.d.ts

@@ -0,0 +1,41 @@
+declare class URL {


Similarly, this file shouldn't be needed either.

martijnwalraven · 2018-06-15T20:55:14Z

packages/apollo-datasource-rest/src/connectors/memcached.ts

+    const version = ++this.logicalClock;
+    const operations: any[] = [];
+    for (const tag of tags) {
+      // what should be a good ttl to set here?


Not sure how Memcache/Redis deal with this, but ideally we wouldn't want tags to ever expire at all.

Setting it to 0 seems to do it: https://github.com/memcached/memcached/wiki/Commands#standard-protocol

we should move these to their own packages in the future, eg `apollo-server-cache-redis`

martijnwalraven · 2018-06-16T04:32:47Z

packages/apollo-datasource-rest/src/connectors/memcached.ts

+    // augment data with tags and version
+    const version = ++this.logicalClock;
+    const payload = {
+      v: version,


Hmmm, interesting! I'm worried keeping a global version instead of a version per tag will become a bottleneck and/or source of concurrency issues, but other people may have a better idea of the impact. I suspect that will depend on the characteristics of the store.

On Memcache, one thing to take into consideration is that the global version key will only be stored on a single node, so that will receive a lot of requests. And if it goes down, you basically lose the ability to validate all data (although the same is true for a subset of the data for per-tag version keys).

martijnwalraven · 2018-06-16T04:33:22Z

packages/apollo-datasource-rest/src/connectors/memcached.ts

+      version = 1;
+      await this.client.set(VERSION_KEY, version + 1, 0);
+    } else {
+      await this.client.incr(VERSION_KEY, 1);


I'm a little confused by the need to increment here, I would expect that to happen only on invalidation. What is the reasoning behind this?

martijnwalraven · 2018-06-16T04:33:46Z

packages/apollo-datasource-rest/src/connectors/memcached.ts

@@ -27,8 +29,17 @@ export default class MemcachedKeyValueCache implements KeyValueCache {
  ): Promise<void> {
    const { ttl, tags } = Object.assign({}, this.defaultSetOptions, options);

+    // get and incr version number


One thing I'd like to avoid is paying the price for keeping a version (or versions) when a particular entry isn't using cache tags. In that case, I don't think we'll want to read (let alone increment) the version at all.

martijnwalraven · 2018-06-16T04:40:26Z

packages/apollo-datasource-rest/src/connectors/memcached.ts

+    if (tags.length !== 0) {
+      const versions = await this.client.getMulti(tags);
+      for (const tag in versions) {
+        if (versions[tag] !== undefined && versions[tag] > payload.v) {


We'd need a way to deal with overflow here, because currently once we wrap around entries will never be valid again.

One benefit of using per-tag versions is that we don't have to depend on ordering. All we need to check is if versions match, and if they do not the entry is invalid. So (I think?) that gets around having to deal with overflow.

martijnwalraven · 2018-06-16T04:44:28Z

packages/apollo-datasource-rest/src/connectors/memcached.ts

@@ -60,11 +73,18 @@ export default class MemcachedKeyValueCache implements KeyValueCache {

  async invalidate(tags: string[]): Promise<void> {
    // set the invalidation "timestamp" using logical clock for every tag
-    const version = ++this.logicalClock;
+    let version = await this.client.get(VERSION_KEY);


I wonder if we can do this in a more efficient way, because checking to see whether the key exists first on every increment seems expensive (and may also run into concurrency issues). Not sure if this is part of the underlying protocol and whether our client supports it, but it seems libmemcached has a memcached_increment_with_initial function.

sebas5384 · 2020-04-15T00:34:06Z

@abernix this PR seems very interesting for imperative purging by cache tags, can't find an explanation on why this code wasn't merged or there's no plan to support cache tags.
Could you please point me in the right direction? I need this feature. Thanks :)

martijnwalraven and others added 4 commits June 12, 2018 10:14

Enable declarationMap in tsconfig.json

74deb28

See http://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-9.html

Add apollo-server-caching package and improve typings

c7da09b

Remove superfluous test steps

081abe1

add KeyValueCache implementations for Memcached and Redis

121bf56

clarencenpy requested a review from martijnwalraven June 13, 2018 12:36

ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jun 13, 2018

promisify client calls so we can use await

fae6330

martijnwalraven reviewed Jun 13, 2018

View reviewed changes

mocked out all side effects so tests run instantly

ba72648

martijnwalraven force-pushed the server-2.0/caching branch 2 times, most recently from edbb2c8 to 395da0f Compare June 14, 2018 18:18

evans reviewed Jun 14, 2018

View reviewed changes

clarencenpy added 4 commits June 14, 2018 14:40

simplified async code

508892c

test that keys are expired correctly

d0f19e4

Merge branch 'server-2.0/caching-connectors' of /Users/clarencenpy/Ap…

c2373aa

…ollo/apollo-server with conflicts.

add support for cache tag invalidation

408edba

clarencenpy force-pushed the server-2.0/caching-connectors branch 2 times, most recently from 88c3a8b to 408edba Compare June 15, 2018 01:59

Merge branch 'server-2.0/caching' into server-2.0/caching-connectors

d48e745

clarencenpy commented Jun 15, 2018

View reviewed changes

martijnwalraven reviewed Jun 15, 2018

View reviewed changes

clarencenpy added 2 commits June 15, 2018 17:05

moved cache connectors to apollo-datasource-rest

e9407cc

we should move these to their own packages in the future, eg `apollo-server-cache-redis`

remove unnecessary types

ba12f29

clarencenpy force-pushed the server-2.0/caching-connectors branch from ca4d9bb to 92bd19a Compare June 16, 2018 00:07

store version key or logical clock in key-value store

1c92359

clarencenpy force-pushed the server-2.0/caching-connectors branch from 92bd19a to 1c92359 Compare June 16, 2018 00:10

martijnwalraven reviewed Jun 16, 2018

View reviewed changes

clarencenpy changed the title ~~Add basic support for Memcached and Redis~~ Add support for cache tag invalidation Jun 18, 2018

ghost added the ⛲️ feature New addition or enhancement to existing solutions label Jun 18, 2018

clarencenpy closed this Jun 19, 2018

abernix deleted the server-2.0/caching-connectors branch February 25, 2020 20:59

This was referenced Jan 17, 2022

[Snyk] Security upgrade node-fetch from 2.6.7 to 3.1.1 k4ny/apollo-server#5

Open

[Snyk] Security upgrade node-fetch from 2.6.7 to 3.1.1 vutting4221/apollo-server#19

Open

github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023

Add support for cache tag invalidation #1169

Add support for cache tag invalidation #1169

Conversation

clarencenpy commented Jun 13, 2018

clarencenpy commented Jun 13, 2018

martijnwalraven left a comment

Choose a reason for hiding this comment

martijnwalraven Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnwalraven Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1ru4l Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

clarencenpy Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarencenpy Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clarencenpy Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

clarencenpy Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnwalraven Jun 16, 2018 • edited Loading

Choose a reason for hiding this comment

sebas5384 commented Apr 15, 2020

martijnwalraven Jun 13, 2018 •

edited

Loading

martijnwalraven Jun 13, 2018 •

edited

Loading

n1ru4l Jun 13, 2018 •

edited

Loading

clarencenpy Jun 13, 2018 •

edited

Loading

clarencenpy Jun 15, 2018 •

edited

Loading

clarencenpy Jun 15, 2018 •

edited

Loading

clarencenpy Jun 15, 2018 •

edited

Loading

martijnwalraven Jun 16, 2018 •

edited

Loading