Refactor cache parameters #225

cecton · 2021-12-01T11:24:19Z

Following our discussion, this PR removes the cache parameters from cli arguments and use environment variable instead. In the future when we know what and how we would like to expose these, we can make a more permanent decision. In the meantime, this will simplify the code as we don't have to carry around more things than schema, configuration and router.

Changes

Remove CLI parameters for cache control
Remove trait WithCaching
Use named cached query planner instead of trait: ApolloRouter should have good defaults by default
Move NaiveIntrospection's spawn_blocking to ApolloRouter's internal
Move cache pre-population to ApolloRouter's internal
Use async_trait for RouterFactory (simplifies mocking in test)
Cache pre-population ran in a background task to not block the ApolloRouter's creation

We can't spawn a task for each and every query but we can start one task for all of them so the ApolloRouter creation is not slowed down by the cache pre-population.

Related to #189

…nternal's The spawn_blocking was introduced in 3ad1827 to make sure the NaiveIntrospection instantiation does not block the local thread.

It needs to be an Arc because it is passed to a tokio::spawn at some point which requires the future to own its values.

We can't spawn a task for each and every query but we can start one task for all of them so the ApolloRouter creation is not slowed down by the cache pre-population.

garypen

I like the fact that we are removing WithCaching. This is a nice simplification.

Geal · 2021-12-01T14:33:11Z

apollo-router/src/apollo_router.rs

+                .expect("NaiveIntrospection instantiation panicked")
+        };
+
+        // Start warming up the cache in a background task


This is smart, but we might lose the benefit of warming the cache up, since it could immediately get some queries that could have been precalculated. If we block on warming up, starting the router is slightly slower, but when reloading configuration there's no delay, we create the router before restarting the server

Good point!

Fixed in 2f34fcb

Geal · 2021-12-01T14:35:50Z

apollo-router/src/apollo_router.rs

+                    for (query, operation, options) in
+                        previous_router.query_planner.get_hot_keys().await
+                    {
+                        // We can ignore errors, since we are just warming up the


(very small nitpick) we have to ignore errors, because some of the queries that were previously in the cache might not work with the new schema

Right, I will fix this, this was copy-pasted

Fixed in 4b16837

Geal · 2021-12-01T14:36:55Z

this makes the code much simpler, nice :)

o0Ignition0o

I really like where this is going, +1 for me.

A couple of nits / questions here and there.

We might wanna make sure we still get tracing and spans (I'm never going to be at ease until we have tests that cover that, especially when we mark functions sync / async and poke around async_trait).

o0Ignition0o · 2021-12-01T14:52:02Z

apollo-router/src/apollo_router.rs

+        // this one is ready.
+        //
+        // We might lose the benefit of warming the cache up, since it could immediately get some
+        // queries that could have been precalculated. If we block on warming up, starting the


I don't understand :/

Do we here mean we are trading of update responsiveness for overall throughput?

"Populate the new router's cache before it starts handling requests. We trade off update responsiveness in favor of request processing latency."

This is related to this discussion: #225 (comment)

What I understand: there is no point of doing this job in a background task because the server is not stopped while the new router is being created. But @Geal thinks we might lose the benefit of warming up the cache. I didn't think about this too much to be honest. Feel free to update the comment to whatever is more clear.

Let me know if this is still a requirement before merging. Otherwise I will merge now

I still don't understand the message, but this by no means should block merging

to explain a bit more: if we first warm up the cache, then switch to the new config, the next queries will benefit from the warmed up cache. While if we switch and warm up in background, the next queries might be blocked until the cache is primed, so there'll be a perf hit

Oh I understand better too now.

Fixed in 37302eb

o0Ignition0o · 2021-12-01T14:52:43Z

apollo-router/src/lib.rs

@@ -561,7 +561,7 @@ impl FederatedServer {
        let state_machine = StateMachine::new(
            server_factory,
            Some(state_listener),
-            ApolloRouterFactory::new(self.plan_cache_limit, self.query_cache_limit),
+            ApolloRouterFactory::new(),


o0Ignition0o · 2021-12-01T14:53:51Z

apollo-router/src/router_factory.rs

-            plan_cache_limit,
-            query_cache_limit,
-        }
+    pub fn new() -> Self {


this looks a lot like Default, which is derived above. Do we have something in mind?

I think you are right. I added new() to cover the parameters. Now they have gone, I think we could remove new() and just use default().

Fixed in f218f49

o0Ignition0o · 2021-12-01T14:57:03Z

thread 'files::tests::basic_watch' panicked at 'assertion failed: futures::poll!(watch.next()).is_pending()', apollo-router/src/files.rs:79:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
``` let's keep a close eye on this, this shouldnt happen.

cecton · 2021-12-02T09:50:04Z

thread 'files::tests::basic_watch' panicked at 'assertion failed: futures::poll!(watch.next()).is_pending()', apollo-router/src/files.rs:79:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
``` let's keep a close eye on this, this shouldnt happen.

where do you see this?

cecton · 2021-12-02T10:10:53Z

We might wanna make sure we still get tracing and spans (I'm never going to be at ease until we have tests that cover that, especially when we mark functions sync / async and poke around async_trait).

I hear you and I feel the same way. Hopefully we will solve this soon.

Jaeger seems to be working alright. BUT I had a harder time to start the server with Jaeger. In the past jaeger was enabled by default but this has been removed somehow. Is this intentional?

cecton · 2021-12-02T10:16:51Z

Jaeger seems to be working alright. BUT I had a harder time to start the server with Jaeger. In the past jaeger was enabled by default but this has been removed somehow. Is this intentional?

Created #232 to address this

o0Ignition0o · 2021-12-02T10:22:11Z

In the past jaeger was enabled by default but this has been removed somehow. Is this intentional?

I think the demo default binds to starstuff, and there is no jaeger receiver, this might be the reason.

#232 does the right thing imo, good job!

o0Ignition0o · 2021-12-02T10:25:45Z

thread 'files::tests::basic_watch' panicked at 'assertion failed: futures::poll!(watch.next()).is_pending()', apollo-router/src/files.rs:79:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
``` let's keep a close eye on this, this shouldnt happen.

where do you see this?

https://app.circleci.com/pipelines/github/apollographql/router/594/workflows/61747ae2-f942-4d91-b581-a59d5d1583cc/jobs/3120 was the failing job, I guess I need to dig further.

cecton added 10 commits December 1, 2021 10:50

Move plan cache limit to runtime env variable

fec6661

Move query cache limit to runtime env variable

6951e51

Remove get_hot_keys() from trait QueryPlanner

439b6b1

Move NaiveIntrospection's spawn_blocking & hot keys to ApolloRouter i…

8a05a6c

…nternal's The spawn_blocking was introduced in 3ad1827 to make sure the NaiveIntrospection instantiation does not block the local thread.

Remove todo

adac30b

It needs to be an Arc because it is passed to a tokio::spawn at some point which requires the future to own its values.

Warm up the cache in background task

f810572

We can't spawn a task for each and every query but we can start one task for all of them so the ApolloRouter creation is not slowed down by the cache pre-population.

Clarify expect

ace0753

Fix tests

f4c0e3b

Clippy fixes

ce156ca

Use async_trait as it also simplifies mocking

25a9f27

cecton requested review from garypen, Geal, o0Ignition0o and BrynCooke December 1, 2021 11:31

cecton marked this pull request as ready for review December 1, 2021 11:31

garypen approved these changes Dec 1, 2021

View reviewed changes

cecton added the size/small label Dec 1, 2021

cecton self-assigned this Dec 1, 2021

Geal reviewed Dec 1, 2021

View reviewed changes

Geal approved these changes Dec 1, 2021

View reviewed changes

cecton added 2 commits December 1, 2021 15:40

More accurate comment

4b16837

Remove unnecessary optimization

2f34fcb

o0Ignition0o approved these changes Dec 1, 2021

View reviewed changes

Remove constructor new in favor of Default

f218f49

cecton added 3 commits December 2, 2021 17:31

Explain better

37302eb

Merge commit ee0d7f9 (no conflict)

2bd4de8

Clippy fixes

438a7f1

cecton merged commit 31eb0d8 into main Dec 3, 2021

cecton deleted the cecton-refactor-cache-parameters branch December 3, 2021 08:37

tinnou pushed a commit to Netflix-Skunkworks/router that referenced this pull request Oct 16, 2023

release: composition@v2.2.3 (apollographql#225)

29f571e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor cache parameters #225

Refactor cache parameters #225

cecton commented Dec 1, 2021 •

edited

Loading

garypen left a comment

Geal Dec 1, 2021

cecton Dec 1, 2021

cecton Dec 1, 2021

Geal Dec 1, 2021

cecton Dec 1, 2021

cecton Dec 1, 2021

Geal commented Dec 1, 2021

o0Ignition0o left a comment

o0Ignition0o Dec 1, 2021

cecton Dec 2, 2021

cecton Dec 2, 2021

o0Ignition0o Dec 2, 2021

Geal Dec 2, 2021

cecton Dec 2, 2021

o0Ignition0o Dec 1, 2021

o0Ignition0o Dec 1, 2021

garypen Dec 1, 2021

cecton Dec 2, 2021

o0Ignition0o commented Dec 1, 2021

cecton commented Dec 2, 2021

cecton commented Dec 2, 2021

cecton commented Dec 2, 2021

o0Ignition0o commented Dec 2, 2021

o0Ignition0o commented Dec 2, 2021

Refactor cache parameters #225

Refactor cache parameters #225

Conversation

cecton commented Dec 1, 2021 • edited Loading

Changes

garypen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geal commented Dec 1, 2021

o0Ignition0o 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

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

o0Ignition0o commented Dec 1, 2021

cecton commented Dec 2, 2021

cecton commented Dec 2, 2021

cecton commented Dec 2, 2021

o0Ignition0o commented Dec 2, 2021

o0Ignition0o commented Dec 2, 2021

cecton commented Dec 1, 2021 •

edited

Loading