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

Refactor model.rs & QueryPlanner related stuff #233

Merged
merged 6 commits into from
Dec 3, 2021
Merged

Conversation

cecton
Copy link
Contributor

@cecton cecton commented Dec 2, 2021

Changes

  • Cleanup snapshots: some snapshots files were unused and some had path not update. I made another PR for this Cleanup snapshots #234
  • Parse planner result as a PlannerResult instead of QueryPlanner: this new PlannerResult enum can detect only the kind QueryPlanner and will return a specific error on parsing if something else is returned by the router bridge.
  • Moved most of the stuff in model.rs to query_planner/mod.rs and make them private
  • Make Selection private and move all the logic around "selection" of the query planner to selection.rs (it's kinda big, it deserves its own file)
  • Moved function select() on Response to selection.rs and make it private
  • Deleted apollo-router/tests/fixtures/query_plan.json because it was not used anymore

Related to #234
Related to #233

@cecton cecton self-assigned this Dec 2, 2021
@cecton cecton mentioned this pull request Dec 2, 2021
@cecton cecton marked this pull request as ready for review December 2, 2021 12:18
@Geal
Copy link
Contributor

Geal commented Dec 2, 2021

could we hold off on big file movements for a while after this PR lands? It has been happening often recently, and it is getting hard to follow, especially if we are trying to modify the same files in other pull requests.

It also does not help for reviews when there are other changes during the move. Like, in this commit 7524bc5, ath the same time there's the change to use QueryPlanResult and deleting the model.rs file.

@cecton cecton mentioned this pull request Dec 2, 2021
@cecton
Copy link
Contributor Author

cecton commented Dec 2, 2021

@Geal all things are interconnected. I made the list of changes I did in the PR description.

@cecton
Copy link
Contributor Author

cecton commented Dec 2, 2021

@Geal it's mostly just moving things around and replacing some pub by pub(crate).

@Geal
Copy link
Contributor

Geal commented Dec 2, 2021

yes I am not saying we should never move things around. But in the past 2 weeks there's been #203 #205 #166 #153 and #140 moving code around. This is a lot, and I wish the code organisation would settle down a bit, because it's hard to review, it's annoying to integrate when we're working concurrently, and going through the code's history and understand past changes is arduous

@cecton
Copy link
Contributor Author

cecton commented Dec 2, 2021

Yes true...

Though #203 is a noop. When I updated my branches after that one got merged, the merge was almost automatic locally. Maybe you are rebasing a lot?

@Geal
Copy link
Contributor

Geal commented Dec 2, 2021

I'm a git monkey, I jump from branch to branch all day 🙈
But also: looking through git history is a very regular task.
And with this team's size it will often happen that multiple people are working on the same files at the same time. I would like you to be mindful of that

@cecton cecton merged commit c759f46 into main Dec 3, 2021
@cecton cecton deleted the cecton-rename-model-rs branch December 3, 2021 13:45
@cecton
Copy link
Contributor Author

cecton commented Dec 3, 2021

Merging this for now as it can conflict easily with other PRs. Don't hesitate to give feedback later

tinnou pushed a commit to Netflix-Skunkworks/router that referenced this pull request Oct 16, 2023
garypen added a commit that referenced this pull request Oct 24, 2023
Cloning the full entry results in leaks.

fixes: #233
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.

3 participants