Warm the query planner cache with hot queries #174

garypen · 2021-11-22T16:02:58Z

If we use an LRU cache (crate: lru) for our query plans, then, whenever
we update queries or configuration, we can pick the top current query
keys and use those to pre-populate a new cache.

Most of this change is just expressing that simple idea.

There are yet questions to be resolved:

How large should the cache be and should it be configurable?
How many hot keys should we preserve on re-configuration?
How best to expose this functionality without affecting good
code structure too much?

Answers:

Cache defaults to 100 items and is configurable
Preserve 20% of hot keys on re-configuration/schema update
Currently passed as parameter, will change to adopt configuration updates

~~Hence this is a draft PR for early review.~~
Now in final review

If we use an LRU cache (crate: lru) for our query plans, then, whenever we update queries or configuration, we can pick the top current query keys and use those to pre-populate a new cache. Most of this change is just expressing that simple idea. There are yet questions to be resolved: - How large should the cache be and should it be configurable? - How many hot keys should we preserve on re-configuration? - How best to expose this functionality without affecting good code structure too much? Hence this is a draft PR for early review.

Make sure to wait for the future which we generate when getting the key of our query plan.

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

crates/apollo-router-core/src/traits.rs

crates/apollo-router/src/graph_factory.rs

As per review comment.

This version avoids the twin evils of: - locking cache for entire duration of delegated get - OR doing multiple delegated get for single query It's more complex, so maybe not desirable on those grounds and does introduce new failure paths and a new crate (bus). If we like the approach, I could probably get rid of the bus crate and do this using condvar or a different spmc implementation.

Slightly better comments help keep things clear.

...from CachingQueryPlanner implementation

I wasn't happy with the bus crate, so this change removes it and makes use of the tokio broadcast channel. Maybe not as fast, but well supported and always good to use less crates. All the mock tests are passing now, but I'm not really happy with the changes I made. Might need more detailed examination.

? I don't know, but I'll remove it again.

So that CircleCI complains less.

cecton

Partially reviewed. I still need to review & understand the entire logic.

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

crates/apollo-router/src/lib.rs

crates/apollo-router/src/router_factory.rs

crates/apollo-router/src/main.rs

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

Most of them at least. I'm leaving the broader configuration and cache handling questions raised by cecile to later work.

Add space to comment at line 172

cecton

So as the discussion about the parameter as been moved, this is not blocking this PR anymore 😁

I still have not finished reviewing the core logic though but I have more remarks about the traits.

.gitignore

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

crates/apollo-router/src/router_factory.rs

cecton · 2021-11-25T10:24:29Z

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

+
+    async fn get_hot_keys(&self) -> Vec<QueryKey> {
+        let locked_cache = self.cached.lock().await;
+        locked_cache
+            .iter()
+            .take(self.plan_cache_limit / 5)
+            .map(|(key, _value)| key.clone())
+            .collect()
    }
 }


This needs to move outside the trait implementation.

The trait QueryPlanner shouldn't be aware of any caching functionality.

Suggested change

async fn get_hot_keys(&self) -> Vec<QueryKey> {

let locked_cache = self.cached.lock().await;

locked_cache

.iter()

.take(self.plan_cache_limit / 5)

.map(|(key, _value)| key.clone())

.collect()

}

}

}

impl<T: QueryPlanner> CachingQueryPlanner<T> {

async fn get_hot_keys(&self) -> Vec<QueryKey> {

let locked_cache = self.cached.lock().await;

locked_cache

.iter()

.take(self.plan_cache_limit / 5)

.map(|(key, _value)| key.clone())

.collect()

}

}

I agree that it may be undesirable, but I think this would break RouterFactory::recreate() because:
let hot_keys = graph.get_query_planner().get_hot_keys().await;
I'm trying to decide if there's a reason why a non-caching QueryPlanner would return hot keys... I mean, it could, but the current implementation doesn't do any tracking and just returns an empty vector.

Yes I just saw... 🤔

I found the API problem behind this issue but it's best to solve it in another PR. So let keep this for now.

o0Ignition0o

The only question I had was about the get_hot_keys function, which we cleared up together, otherwise this looks good to me overall.

The conversations seem to be more about nits, which can be iterated on IMO

And put it in my global .gitignore

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

cecton · 2021-11-25T11:13:55Z

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

+
+    async fn get_hot_keys(&self) -> Vec<QueryKey> {
+        let locked_cache = self.cached.lock().await;
+        locked_cache
+            .iter()
+            .take(self.plan_cache_limit / 5)
+            .map(|(key, _value)| key.clone())
+            .collect()
    }
 }


Yes I just saw... 🤔

crates/apollo-router/src/router_factory.rs

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

Make sure to only drop the cache lock after acquiring the wait lock. Also address more review comments.

cecton

Some small knits but good to go 🤗 Thanks a lot for your patience!!

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

crates/apollo-router/src/router_factory.rs

cecton · 2021-11-25T13:20:51Z

crates/apollo-router-core/src/query_planner/caching_query_planner.rs

+
+    async fn get_hot_keys(&self) -> Vec<QueryKey> {
+        let locked_cache = self.cached.lock().await;
+        locked_cache
+            .iter()
+            .take(self.plan_cache_limit / 5)
+            .map(|(key, _value)| key.clone())
+            .collect()
    }
 }


I found the API problem behind this issue but it's best to solve it in another PR. So let keep this for now.

More review comments

garypen · 2021-11-25T13:37:32Z

Some small knits but good to go 🤗 Thanks a lot for your patience!!

Thanks for your diligent review. I think the quality is much improved.

I've added the requested changes.

garypen requested a review from Geal November 22, 2021 16:02

Remember to wait for our futures

1a94e3c

Make sure to wait for the future which we generate when getting the key of our query plan.

Geal previously requested changes Nov 22, 2021

View reviewed changes

crates/apollo-router-core/src/query_planner/caching_query_planner.rs Show resolved Hide resolved

crates/apollo-router-core/src/traits.rs Show resolved Hide resolved

crates/apollo-router/src/graph_factory.rs Outdated Show resolved Hide resolved

garypen added 8 commits November 23, 2021 08:22

Reinstate the spawn_blocking() call for FederatedGraph::new()

ffb96fb

As per review comment.

Improve comments around caching code

9929415

Slightly better comments help keep things clear.

Merge branch 'main' into warm-query-planner

9af4108

Remove unneeded 'static

36b7be1

...from CachingQueryPlanner implementation

Merge branch 'main' into warm-query-planner

25edd5e

Why did git add this delete file back during the merge

557c5b2

? I don't know, but I'll remove it again.

garypen requested review from cecton, o0Ignition0o and BrynCooke November 24, 2021 17:23

garypen marked this pull request as ready for review November 24, 2021 17:24

Update licenses.html

69cb0e2

So that CircleCI complains less.

cecton suggested changes Nov 24, 2021

View reviewed changes

Geal reviewed Nov 24, 2021

View reviewed changes

crates/apollo-router-core/src/query_planner/caching_query_planner.rs Outdated Show resolved Hide resolved

Geal reviewed Nov 24, 2021

View reviewed changes

crates/apollo-router-core/src/query_planner/caching_query_planner.rs Outdated Show resolved Hide resolved

cecton reviewed Nov 24, 2021

View reviewed changes

crates/apollo-router-core/src/query_planner/caching_query_planner.rs Outdated Show resolved Hide resolved

garypen added 2 commits November 25, 2021 09:07

Address review comments

f31ac3a

Most of them at least. I'm leaving the broader configuration and cache handling questions raised by cecile to later work.

Fix small typo

d880799

Add space to comment at line 172

garypen requested review from Geal and cecton November 25, 2021 09:38

garypen added 2 commits November 25, 2021 09:40

Merge branch 'main' into warm-query-planner

e7b00c0

Merge branch 'main' into warm-query-planner

6c23d47

cecton suggested changes Nov 25, 2021

View reviewed changes

o0Ignition0o approved these changes Nov 25, 2021

View reviewed changes

Remove ctags entry from .gitignore

0d3fd8b

And put it in my global .gitignore

cecton suggested changes Nov 25, 2021

View reviewed changes

Fix potential race when waiting for broadcast

0c8421c

Make sure to only drop the cache lock after acquiring the wait lock. Also address more review comments.

cecton approved these changes Nov 25, 2021

View reviewed changes

Implement review comments

fd1b710

More review comments

garypen merged commit 5a3e34c into main Nov 25, 2021

garypen deleted the warm-query-planner branch November 25, 2021 13:50

garypen linked an issue Nov 25, 2021 that may be closed by this pull request

warm the query planner cache with well known queries #112

Closed

garypen mentioned this pull request Nov 25, 2021

warm the query planner cache with well known queries #112

Closed

Geal mentioned this pull request Dec 13, 2021

request coalescing / query deduplication on subgraph requests #264

Closed

garypen mentioned this pull request Dec 14, 2021

Faster Caching #42

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warm the query planner cache with hot queries #174

Warm the query planner cache with hot queries #174

garypen commented Nov 22, 2021 •

edited

cecton left a comment

cecton left a comment

cecton Nov 25, 2021

garypen Nov 25, 2021

cecton Nov 25, 2021

cecton Nov 25, 2021

o0Ignition0o left a comment

cecton Nov 25, 2021

cecton left a comment

cecton Nov 25, 2021

garypen commented Nov 25, 2021

Warm the query planner cache with hot queries #174

Warm the query planner cache with hot queries #174

Conversation

garypen commented Nov 22, 2021 • edited

cecton left a comment

Choose a reason for hiding this comment

cecton left a comment

Choose a reason for hiding this comment

cecton Nov 25, 2021

Choose a reason for hiding this comment

garypen Nov 25, 2021

Choose a reason for hiding this comment

cecton Nov 25, 2021

Choose a reason for hiding this comment

cecton Nov 25, 2021

Choose a reason for hiding this comment

o0Ignition0o left a comment

Choose a reason for hiding this comment

cecton Nov 25, 2021

Choose a reason for hiding this comment

cecton left a comment

Choose a reason for hiding this comment

cecton Nov 25, 2021

Choose a reason for hiding this comment

garypen commented Nov 25, 2021

garypen commented Nov 22, 2021 •

edited