-
Notifications
You must be signed in to change notification settings - Fork 240
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 Support For Automatic Persisted Queries #187
Conversation
I'm not convinced by this implementation. How do you plan to scale this horizontally? The queries will be stored only on one peer. |
Interesting point - I think this is how Apollo Server implemented it, which is why I went down this road, then generalised the way the query is delivered. The argument in favour would be that the cost of hashing a query the first time an instance sees it will still massively reduce the number of times the full query is sent over the wire. Really, this entire interface or setting might be better off merged into the existing persisted queries solution, in a customisable manner, since there is no firm 'Best Practice', such that there is: Persisted Query Settings:
Example implementations would be: Current Persisted Queries (Ahead of time):
Apollo Client Compatible Automatic Persisted Queries (Runtime):
Cached Apollo Client Compatible Automatic Persisted Queries (Runtime + Shared Cache):
|
My point is that the current interface (ahead of time) cannot be made dynamic without adding support for multiple instances of the server that store the query. Otherwise subsequent requests that hit a different server would not have the cached query. In the code in this PR the “get” operation is synchronous and it should be made async (and likely moved in a common interface). In your list, the Persisted Query Settings:
I’m also skeptical that adding a delay of a round trip to Redis is going to improve latency much in practice. We are trading the cost of sending a few hundred bytes from the client to fetching those from Redis. Given how HTTP/1.1 keepalive and TCP work, sending a few hundreds bytes more should have no impact. Notably this makes a lot of sense if our query is massive. |
I agree with your reasoning that it'd need to be async. As a general rule, any persisted query requires that the payload reduction savings are greater than the lookup delay. How big the lookup delay is depends on the implementation. My understanding is that: Ahead of Time Persisted Queries (AoTPQ):
Automatic Persisted Queries (APQ):
Cached APQ:
Appreciate the responsiveness here, but I also want to flag that in order for us to migrate to fastify-gql, we need to support APQ as otherwise we'll lose backwards compatibility - obviously that doesn't mean that it's something fastify-gql should support - so if it's a no-go, we can fork. |
Could you confirm that you are using "sticky sessions" for APQ? How can you guarantee that the same client goes to the same server otherwise? What i do not understand about APQ is if it's actually feasible in production. It seems very likely that a client talking to a different server might not get the cached one.
Then we are good, can you update the PR?
I'm cautious about adding feature, I think this is legit and we should add it in one form or another. |
I will do updates to both PR's tomorrow.
Our use case is as follows: Client:
Server:
I hope this (very rough) diagram can covey the idea slightly better: Sticky sessions would ensure that the cache of the machine returning |
Thanks, it makes sense now. Please add a short version of that in the README. |
I've reworked the API & tidied things up a lot based on the conversation above.
|
Can you rebase this on top of |
Have merged the other changes in, diff should be clean. Thanks. |
Minor fixes have been completed. Don't mind the long process - happy to get the prompt feedback, and I appreciate it's a non trivial set of changes. Should other casing also be updated?
|
README.md
Outdated
* `onlyPersisted`: Boolean. Flag to control whether to allow graphql queries other than persisted. When `true`, it'll make the server reject any queries that are not present in the `persistedQueries` option above. It will also disable any ide available (playground/graphiql). | ||
* `persistedQueries`: A hash/query map to resolve the full query text using it's unique hash. Overrides `persistedQuerySettings`. | ||
* `onlyPersisted`: Boolean. Flag to control whether to allow graphql queries other than persisted. When `true`, it'll make the server reject any queries that are not present in the `persistedQueries` option above. It will also disable any ide available (playground/graphiql). Requires `persistedQueries` to be set, and overrides `persistedQuerySettings`. | ||
* `persistedQuerySettings` |
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.
persistedQueryProvider
maybe?
@pscarey any updates on this? It would be great to land! |
@mcollina Sorry about the delay - was away for a few days. Will be updating shortly. |
All updated. FYI, we've been running this in production for over a week, and have had no issues. |
Have implemented the feedback, but it looks like some changes from |
Yes, we are fixing them today, unfortunately something broke in #147 |
I have the reverted the offending commit on master. |
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.
lgtm
Adds customisable support for Automatic Persisted Queries, with default settings compatible with
apollo-client
.#110 already has discussion on this. We required support internally, so it's up to maintainers if support in this way is appropriate. Happy to make tweaks based on feedback if required.
Note this builds on #186.