-
Notifications
You must be signed in to change notification settings - Fork 346
feat!: sunset gql-executor in favor of graphql-yoga #11077
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
Conversation
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
I worry that graphql-ws is a one-man project. It's only his author coding and we don't control it at all. Maybe even with that, it's worth to switch from our custom thing to this, but I wanted to share my concern. |
totally valid! it is just one person, but it seems to be pretty stable as it's the defacto solution for https://github.com/graphql-hive/graphql-yoga/graphs/contributors. Then again, graphql-yoga only has 3 contributors 😅 . |
Signed-off-by: Matt Krick <[email protected]>
I mean, if you think it's the best we can get, let's go with it and hope it doesn't stop or drift from what we need 🤞🏼 curious: why are we using graphql-ws instead of graphql-yoga? I haven't check them so this question might be stupid 😅 |
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
This looks GREAT! Thanks for the work man, this is huge. Some notes and questions:
|
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
sure!
when the new version is deployed they must upgrade their client to the new version. should happen automatically, if it doesn't they'll get an error & when they hit refresh everything will be good
yep!
yep
yeah, the ID doesn't matter, as long as they're all unique
good call! yep. the shape of the payload also changes. you must remove
chronos now talks HTTP via public URL (open to changing that? but that seems difficult... the busiest cronjob fires once every 5 mins, i think we'll be OK if our public endpoint gets 1-2 extra requests every 5 mins 😅 Embedder talks via postgres, so no change necessary
Here's my hot take-- readiness CAN include upstream dependencies, liveness CANNOT. if a server can't connect to PG, we want to take it out of rotation (ie don't serve it traffic), but we shouldn't kill it immediately. maybe the DB is down, maybe it is under heavy load. the readiness probe will still fire & if it comes back up, then we have a pre-warmed container ready to go. From the docs:
https://elastisys.com/the-difference-between-liveness-and-readiness-probes-in-kubernetes/ |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Thanks for the answers. Good to know that releasing shouldn't be a problem for clients. I was just wondering what might happen with users connected to old web servers, that were working with old gql-ex, then an user on that web server disconnects and connects to a new web server and no gql-ex. Ok about Chronos. Not ideal, but not that bad neither. It won't work on Staging, but we are not using Chronos on Staging for now so, problem for the future 😅 if ever. And I can agree on the readiness depending on some external, but it isn't ideal either. Imagine Postgres down for some time due to a maintenance or an unexpected error on GCP side, we would remove all servers from the load balancer, throwing a very very ugly error to our users, instead of some pretty thing saying "hey, our app is having backend problems, we will be back soon". That said, as it shouldn't happen often, let's keep it as you coded it for now. I'm not a fan of external dependencies for any of those, but on the readiness after startup is a fair debate. |
Ah, one more thing: IMHO this should be a major release. We are changing the architecture of the application and how it has to be deployed. This is impacting for all our self-hosted clients, we should make it easy for them to see we changed something important. It's easy to do, just changing the PR's title to |
Signed-off-by: Matt Krick <[email protected]>
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Description
fix #11065
This PR is big. I'm sorry. Here's a video walkthrough of the changes: https://www.loom.com/share/6905c3b988e946bb9e3341f54747662b
What it does: