-
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
Experimental: Introduce a pool of query planners #4897
Conversation
This comment has been minimized.
This comment has been minimized.
CI performance tests
|
This changes adresses contention we didn't see before the pool of planner was introduced. Borrow semantics and locks make for a surprising pattern where a lock is held a bit too long. This changeset adresses it, and we expect a performance boost at stale / under heavy load.
*Description here* Fixes #**issue_number** <!-- start metadata --> --- **Checklist** Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review. - [ ] Changes are compatible[^1] - [ ] Documentation[^2] completed - [ ] Performance impact assessed and acceptable - Tests added and passing[^3] - [ ] Unit Tests - [ ] Integration Tests - [ ] Manual Tests **Exceptions** *Note any exceptions here* **Notes** [^1]: It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. [^2]: Configuration is an important part of many changes. Where applicable please try to document configuration examples. [^3]: Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. Co-authored-by: Marc-Andre Giroux <mgiroux@netflix.com>
*Description here* Fixes #**issue_number** <!-- start metadata --> --- **Checklist** Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review. - [ ] Changes are compatible[^1] - [ ] Documentation[^2] completed - [ ] Performance impact assessed and acceptable - Tests added and passing[^3] - [ ] Unit Tests - [ ] Integration Tests - [ ] Manual Tests **Exceptions** *Note any exceptions here* **Notes** [^1]: It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. [^2]: Configuration is an important part of many changes. Where applicable please try to document configuration examples. [^3]: Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. --------- Co-authored-by: Marc-Andre Giroux <mgiroux@netflix.com>
Co-authored-by: Edward Huang <edward.huang@apollographql.com>
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.
could you mention it in the docs?
I am not sure auto
will be the best option here. Tokio already spawns threads according to the available capacity, so if at the same time there are as many planner threads as there are cores, then we risk not having any capacity left to handle requests because the planners do blocking work
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 seems to be different to the approach of using web workers that you were discussing earlier in the week. Was there a problem with that approach? I'm asking because I'm curious, but I think this is a better approach to that anyway.
One other thing worth asking. Instead of managing a queue using async_channel
, maybe put some kind of adaptive, load-shedding queuing model in front of the pool? e.g.: https://crates.io/crates/little-loadshedder and expose the configuration so that user's can express a queue wait time in configuration?
It turns out both approaches are equivalent in term of runtime capabilities. Thankfully a v8 runtime is initialized in a static :D The good news is we should be able to drop in any planner implementation in the future.
This could be worth considering as a followup. I'm fairly happy with the MPMC approach since workers decide to pick new jobs as soon as they're ready to deal with them. |
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.
Needs a config metric.
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.
Does anyone else see this warning:
garypen@Garys-MacBook-Pro router % cargo check
warning: /Users/garypen/dev/router/apollo-router/Cargo.toml: file `/Users/garypen/dev/router/apollo-router/benches/planner.rs` found to be present in multiple build targets:
* `example` target `planner`
* `bench` target `planner`
<etc...>
Maybe address this? I don't know if it really matters, but ...
@garypen this is odd, I don't see several targets in the |
The warning is removed if you add:
to the package section in I think you need to decide if this is the behaviour you want wrt other benches/examples etc... |
Docs for query planner pool (#4897)
Experimental: Introduce a pool of query planners (PR #4897)
The router supports a new experimental feature: a pool of query planners to parallelize query planning.
You can configure query planner pools with the
supergraph.query_planner.experimental_available_parallelism
option:Its value is the number of query planners that run in parallel, and its default value is
1
. You can set it to the special valueauto
to automatically set it equal to the number of available CPUs.You can discuss and comment about query planner pools in this GitHub discussion.
By @xuorig and @o0Ignition0o in #4897