chore(federation): add unit tests and documentation for conditions handling#6325
Merged
goto-bus-stop merged 6 commits intodevfrom Nov 22, 2024
Merged
chore(federation): add unit tests and documentation for conditions handling#6325goto-bus-stop merged 6 commits intodevfrom
goto-bus-stop merged 6 commits intodevfrom
Conversation
goto-bus-stop
requested review from
SimonSapin,
TylerBloom,
dariuszkuc,
duckki,
lrlna and
sachindshinde
as code owners
Collaborator
✅ Docs Preview ReadyNo new or changed pages found. |
Contributor
|
@goto-bus-stop, please consider creating a changeset entry in |
goto-bus-stop
commented
Nov 22, 2024
| Boolean(bool), | ||
| } | ||
|
|
||
| impl Display for Conditions { |
Member
Author
There was a problem hiding this comment.
This is mostly for use in tests, though it might help with debugging as well
|
CI performance tests
|
goto-bus-stop
commented
Nov 22, 2024
| .node_weight_mut(node_index) | ||
| .ok_or_else(|| FederationError::internal("Node unexpectedly missing"))?; | ||
| let conditions = handled_conditions.update_with(&node.selection_set.conditions); | ||
| let conditions = node.selection_set.conditions.update_with(&handled_conditions); |
Member
Author
There was a problem hiding this comment.
Only usage site of update_with().
lrlna
approved these changes
Nov 22, 2024
Member
lrlna
left a comment
lrlna
left a comment
There was a problem hiding this comment.
I like always and never as an api to conditions!
goto-bus-stop
added a commit
that referenced
this pull request
Nov 22, 2024
While I was working on #6325, I figured I might as well tackle this related issue quickly. Certain graph and query shapes cause the `FetchSelectionSet::conditions` to be recomputed very, very frequently. Now, they are only recomputed when required. On most plans there is no noteworthy difference. When `OnceLock::get_or_try_init` is stabilised, this will be easier :) <!-- [ROUTER-484] -->
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
Conditionsstructure represents the combined skip/include conditions of a GraphQL query path (and of a FetchDependencyGraphNode).The storage of variable conditions looks inefficient. I tried some quick refactors to make it more efficient, which didn't hugely affect overall planning performance, even on plans that spend a lot of time computing conditions. Perhaps the representation isn't the biggest issue. I think we can still simplify it, though. I might tinker with that if I find some spare cycles in December.
This PR does not change the representation. I only added tests and documentation.
I also flipped the order of the
update_with()argument. The order of the arguments disagreed with the name of the method. We were not updating the argument at all: we were updatingself, removing conditions already handled by the argument.