Skip to content
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

Add ability for router to deal with query plans with contextual rewrites #5097

Merged
merged 78 commits into from May 23, 2024

Conversation

clenfest
Copy link
Contributor

@clenfest clenfest commented May 7, 2024

Two main things that we're doing in this PR.

  1. We've added a variable to FetchNode called context_rewrites. This is a vector of DataRewrite::KeyRenamer that are specifically taking data from their path (which will be relative and can traverse up the data path) and writes the data into an argument that is passed to the selection set.
  2. There are two cases. In the most straightforward, the data that is passed to the selection set is the same for every entity. This case is pretty easy and doesn't require any special handling. In the second case, the value of the variable may be different per entity. If that is true, we need to use aliasing and duplication in our query in order to send it to subgraphs. Once Finding the right Batching Mechanism graphql/composite-schemas-spec#25 is decided and has subgraph support, this query cloning will be able to go away.

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Tests added and passing3
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

  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.

@router-perf
Copy link

router-perf bot commented May 7, 2024

CI performance tests

  • step - Basic stress test that steps up the number of users over time
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • large-request - Stress test with a 1 MB request payload
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • xxlarge-request - Stress test with 100 MB request payload
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • xlarge-request - Stress test with 10 MB request payload
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • no-graphos - Basic stress test, no GraphOS.
  • reload - Reload test over a long period of time at a constant rate of users
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • const - Basic stress test that runs with a constant number of users

This comment has been minimized.

@clenfest clenfest marked this pull request as ready for review May 13, 2024 06:18
@clenfest clenfest requested a review from dariuszkuc as a code owner May 13, 2024 06:18
Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking a lot better than earlier in the week. Great work!

My main interests now are in checking what happens if a subgraph returns an error and the assumptions about which operation to use in build_operation_with_aliasing().

apollo-router/src/query_planner/fetch.rs Outdated Show resolved Hide resolved
apollo-router/src/query_planner/mod.rs Outdated Show resolved Hide resolved
apollo-router/src/query_planner/fetch.rs Outdated Show resolved Hide resolved
apollo-router/src/query_planner/subgraph_context.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@garypen garypen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work on this and thanks for adding the extra tests and cleanup.

I have one comment about alpha federation release, which I don't think we've done before in the router, but apart from that I'm happy.

@@ -188,7 +188,7 @@ regex = "1.10.3"
reqwest.workspace = true

# note: this dependency should _always_ be pinned, prefix the version with an `=`
router-bridge = "=0.5.21+v2.7.5"
router-bridge = "=0.5.23+v2.8.0-alpha.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we happy releasing a router which is using a federation alpha release? Is the intent to update this before we release the next version of the router?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is just to let us bake in a bit more testing before we officially release 2.8.0, but if all goes well we can just promote it. We'll certainly release and bump the pin before we cut a 1.48.0 rc

@o0Ignition0o o0Ignition0o enabled auto-merge (squash) May 23, 2024 09:24
@o0Ignition0o o0Ignition0o merged commit 5a0826e into dev May 23, 2024
13 of 14 checks passed
@o0Ignition0o o0Ignition0o deleted the clenfest/set_context_working branch May 23, 2024 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants