-
Notifications
You must be signed in to change notification settings - Fork 272
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
Refactor cache parameters #225
Changes from 12 commits
fec6661
6951e51
439b6b1
8a05a6c
adac30b
f810572
ace0753
f4c0e3b
ce156ca
25a9f27
4b16837
2f34fcb
f218f49
37302eb
2bd4de8
438a7f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎉 |
||
); | ||
let (shutdown_sender, shutdown_receiver) = oneshot::channel::<()>(); | ||
let result = spawn(async { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,113 +1,45 @@ | ||
use crate::apollo_router::{ApolloPreparedQuery, ApolloRouter}; | ||
use crate::configuration::Configuration; | ||
use crate::http_service_registry::HttpServiceRegistry; | ||
use apollo_router_core::prelude::{graphql::*, *}; | ||
use futures::prelude::*; | ||
use apollo_router_core::prelude::*; | ||
use std::sync::Arc; | ||
|
||
/// Factory for creating graphs. | ||
/// | ||
/// This trait enables us to test that `StateMachine` correctly recreates the ApolloRouter when | ||
/// necessary e.g. when schema changes. | ||
//#[cfg_attr(test, automock)] | ||
#[async_trait::async_trait] | ||
pub(crate) trait RouterFactory<Router, PreparedQuery> | ||
where | ||
Router: graphql::Router<PreparedQuery>, | ||
PreparedQuery: graphql::PreparedQuery, | ||
{ | ||
fn create( | ||
async fn create( | ||
&self, | ||
configuration: &Configuration, | ||
schema: Arc<graphql::Schema>, | ||
plan_cache_limit: usize, | ||
query_cache_limit: usize, | ||
) -> future::BoxFuture<'static, Router>; | ||
fn recreate( | ||
&self, | ||
router: Arc<Router>, | ||
configuration: &Configuration, | ||
schema: Arc<graphql::Schema>, | ||
plan_cache_limit: usize, | ||
query_cache_limit: usize, | ||
) -> future::BoxFuture<'static, Router>; | ||
fn get_plan_cache_limit(&self) -> usize; | ||
fn get_query_cache_limit(&self) -> usize; | ||
previous_router: Option<Arc<Router>>, | ||
) -> Router; | ||
} | ||
|
||
#[derive(Default)] | ||
pub(crate) struct ApolloRouterFactory { | ||
plan_cache_limit: usize, | ||
query_cache_limit: usize, | ||
} | ||
pub(crate) struct ApolloRouterFactory {} | ||
|
||
impl ApolloRouterFactory { | ||
pub fn new(plan_cache_limit: usize, query_cache_limit: usize) -> Self { | ||
Self { | ||
plan_cache_limit, | ||
query_cache_limit, | ||
} | ||
pub fn new() -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this looks a lot like Default, which is derived above. Do we have something in mind? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in f218f49 |
||
Self {} | ||
} | ||
} | ||
|
||
#[async_trait::async_trait] | ||
impl RouterFactory<ApolloRouter, ApolloPreparedQuery> for ApolloRouterFactory { | ||
fn create( | ||
async fn create( | ||
&self, | ||
configuration: &Configuration, | ||
schema: Arc<graphql::Schema>, | ||
plan_cache_limit: usize, | ||
query_cache_limit: usize, | ||
) -> future::BoxFuture<'static, ApolloRouter> { | ||
previous_router: Option<Arc<ApolloRouter>>, | ||
) -> ApolloRouter { | ||
let service_registry = HttpServiceRegistry::new(configuration); | ||
tokio::task::spawn_blocking(move || { | ||
ApolloRouter::new( | ||
Arc::new( | ||
graphql::RouterBridgeQueryPlanner::new(Arc::clone(&schema)) | ||
.with_caching(plan_cache_limit), | ||
), | ||
Arc::new(service_registry), | ||
schema, | ||
query_cache_limit, | ||
) | ||
}) | ||
.map(|res| res.expect("ApolloRouter::new() is infallible; qed")) | ||
.boxed() | ||
} | ||
|
||
fn recreate( | ||
&self, | ||
router: Arc<ApolloRouter>, | ||
configuration: &Configuration, | ||
schema: Arc<graphql::Schema>, | ||
plan_cache_limit: usize, | ||
query_cache_limit: usize, | ||
) -> future::BoxFuture<'static, ApolloRouter> { | ||
let factory = self.create(configuration, schema, plan_cache_limit, query_cache_limit); | ||
|
||
Box::pin(async move { | ||
// Use the "hot" entries in the supplied router to pre-populate | ||
// our new router | ||
let new_router = factory.await; | ||
let hot_keys = router.get_query_planner().get_hot_keys().await; | ||
// It would be nice to get these keys concurrently by spawning | ||
// futures in our loop. However, these calls to get call the | ||
// v8 based query planner and running too many of these | ||
// concurrently is a bad idea. One for the future... | ||
for key in hot_keys { | ||
// We can ignore errors, since we are just warming up the | ||
// cache | ||
let _ = new_router | ||
.get_query_planner() | ||
.get(key.0, key.1, key.2) | ||
.await; | ||
} | ||
new_router | ||
}) | ||
} | ||
|
||
fn get_plan_cache_limit(&self) -> usize { | ||
self.plan_cache_limit | ||
} | ||
|
||
fn get_query_cache_limit(&self) -> usize { | ||
self.query_cache_limit | ||
ApolloRouter::new(Arc::new(service_registry), schema, previous_router).await | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if this is still a requirement before merging. Otherwise I will merge now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand the message, but this by no means should block merging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I understand better too now.
Fixed in 37302eb