Breaking: remove the query_planner service #1552
Conversation
This comment has been minimized.
This comment has been minimized.
o0Ignition0o
requested review from
SimonSapin,
Geal,
garypen,
abernix,
bnjjj and
BrynCooke
o0Ignition0o
changed the title
| @@ -41,6 +42,11 @@ impl BridgeQueryPlanner { | |||
| introspection: Option<Arc<Introspection>>, | |||
| configuration: Arc<Configuration>, | |||
| ) -> Result<Self, QueryPlannerError> { | |||
| // The variables deduplication parameter lives in the traffic_shaping section of the config | |||
| let deduplicate_variables = configuration | |||
| .plugin_configuration("apollo.traffic_shaping") | |||
There was a problem hiding this comment.
could this be a (static) method of the traffic shaping plugin? To notice it easily if something changes in that plugin
There was a problem hiding this comment.
hmmm not really sure how, but there's probably a way for me to ask traffic_shaping to give me its settings once i ve passed a configuration in Oo
There was a problem hiding this comment.
Should this config really be in traffic_shaping?
There was a problem hiding this comment.
something simple like:
impl TrafficShaping {
fn get_configuration_deduplicate_variables(configuration: &Configuration) -> bool {
configuration
.plugin_configuration("apollo.traffic_shaping").map(|conf| conf.get("deduplicate_variables") == Some(&serde_json::Value::Bool(true)))
.unwrap_or_default()
}
}Just to have the code related to this part of the configuration in the plugin so we don't miss something if config options change
There was a problem hiding this comment.
Should this config really be in traffic_shaping?
I don't think so, while its goal is to reduce the size of subgraph requests and responses, it's more related to query plan execution
There was a problem hiding this comment.
I see.
RE: traffic_shaping yml config: I think we should revisit this as part of #1500
There was a problem hiding this comment.
fixed in 5b5612a
|
removing code feels nice :) |
| @@ -41,6 +42,11 @@ impl BridgeQueryPlanner { | |||
| introspection: Option<Arc<Introspection>>, | |||
| configuration: Arc<Configuration>, | |||
| ) -> Result<Self, QueryPlannerError> { | |||
| // The variables deduplication parameter lives in the traffic_shaping section of the config | |||
| let deduplicate_variables = configuration | |||
| .plugin_configuration("apollo.traffic_shaping") | |||
There was a problem hiding this comment.
Should this config really be in traffic_shaping?
|
I guess the team already discussed and agreed on removal, just want to highlight that it was at one point thought that query plan caching could eventually be configurable via plugin. I think this'll close down this opportunity. Was the option of introducing a plugin internal trait discussed. This would allow us to keep things clean architecturally. Thinking specifically of query dedup here. |
Yes it was, and we're still very much open to adding one if needs be! I'd be very happy to discuss it next week. |
|
What was supposed to be modifiable by a plugin in plan caching? There's nothing preventing us from introducing new plugin hook points in the future. Trying to leverage the plugin architecture is a good goal because it allows to separate concerns cleanly, but it does not make sense for every feature. Caching and query deduplication in particular should be implemented as part of the underlying service, or as a layer above a clonable service like SubgraphService. Because if it is done as a plugin, it forces us to reintroduce a Buffer and the related reliability issues. |
Fixes #1545