-
Notifications
You must be signed in to change notification settings - Fork 2.8k
cache remote schema introspection query response (fix #1679) #2089
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
cache remote schema introspection query response (fix #1679) #2089
Conversation
TODO:- Add 'reload_remote_schema' query type
|
Deploy preview for hasura-docs ready! Built with commit bfba275 |
|
Review app for commit d5c48e8 deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
|
Review app for commit 6fd7e4f deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
|
Review app for commit cd6b659 deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
Resolve Conflicts: server/src-lib/Hasura/RQL/DDL/Schema/Table.hs server/src-lib/Hasura/Server/App.hs
|
Review app for commit a452f07 deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
Resolve Conflicts: docs/graphql/manual/api-reference/schema-metadata-api/index.rst docs/graphql/manual/api-reference/schema-metadata-api/syntax-defs.rst server/src-lib/Hasura/RQL/Types/SchemaCache.hs server/src-lib/Hasura/Server/App.hs server/src-lib/Hasura/Server/Query.hs
|
Review app for commit ac480f9 deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
|
Review app for commit 5d96397 deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
ecthiender
left a comment
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.
Server changes LGTM. Requires console changes.
|
@karthikvt26 @rikinsk For console, the button highlighted 👇 (and the help text) need to change to use |
|
/heroku deploy |
|
Review app for commit fe1ab51 deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
|
Review app for commit 6d7039b deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
|
Review app for commit dc4c026 deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
|
Review app for commit bfba275 deployed to Heroku: https://hge-ci-pull-2089.herokuapp.com |
|
Review app https://hge-ci-pull-2089.herokuapp.com is deleted |
Description
This PR fixes:-
200s) as error message to the clientDetails:-
/v1/querywhich modifies the schema state, such astrack_table,track_functionetc.add_remote_schemaquery type) makes 2 network requests. One to fetch and validate. The other is to fetch and stitch the remote schema at different levels.bulkquery takes significant time since each query make a network request to remote server and rebuilds the graphql schemaThe above issues are solved by caching remote schema GraphQL types and avoid refreshing remote schema for each
/v1/queryAPI call.To refresh remote schema, a new
/v1/queryquery typereload_remote_schemais introduced and it is integrated withreloadbutton undermodifysection of a remote schema in console.Results:-
Tracking ~300 tables through console which actually makes a bulk query, in the local setup.
Affected components
Design and Solution
Server:-
GraphQL typesfrom remote server introspection query response to avoid making network requests on every metadata update/v1/query.runQueryMfunction. This'll abort any changes made if graphql schema generation failsreload_remote_schemaquery type to re-fetchGraphQL typesfrom a given remote server.200status codeinternalfield oferrorif it occurs while contacting remote server to fetch schemaConsole:-
reload_remote_schemaAPI instead ofreload_metadataunderreloadsection of a remote schema.To Reviewer (server):-
Start with
../SchemaCache.hsfile and look forRemoteSchemaCtxtypeRemoteGCtxmoved to..GraphQL/Context.hssince it represents graphQLcontextfor remote schemaRemoteSchemaCtxis context of remote schema which'll be stored in schema cachebuildGCtxMapPGandbuildGCtxMapare defined to make GraphQL schema in state monadcritical modules:
../Server/Query.hs../App.hs../DDL/RemoteSchema.hs../GraphQL/RemoteServer.hsRelated Issues
close #1679
Breaking Changes
errorfield on non200status codeBefore
{ "path": "$.args[0].args", "error": "<Large HTML Response Body from Remote Server>", "code": "remote-schema-error" }After:
{ "internal": { "raw_body": "<Large HTML Response Body from Remote Server>" }, "path": "$.args[0].args", "error": "introspection query to https://hasura.io/random has responded with 404 status code", "code": "remote-schema-error" }http-logof the server will have a newinternalproperty inerrorobject when HTTP or API exception from remote server occurs while adding.Before:
{ "timestamp": "2019-06-19T18:59:18.060+0530", "level": "info", "type": "http-log", "detail": { "status": 400, "query_hash": "ddc917a7d389e0b2bf2ec4e566df8c5a1f3cc612", "http_version": "HTTP/1.1", "query_execution_time": 0.001634484, "request_id": null, "url": "/v1/query", "ip": "127.0.0.1", "response_size": 147, "user": { "x-hasura-role": "admin" }, "method": "POST", "detail": { "error": { "path": "$.args[0].args", "error": "HTTP exception occurred while sending the request to http://localhost:8181/graphql", "code": "remote-schema-error" }, "request": "{\"type\":\"bulk\",\"args\":[{\"type\":\"add_remote_schema\",\"args\":{\"name\":\"random\",\"definition\":{\"url\":\"http://localhost:8181/graphql\",\"headers\":[],\"forward_client_headers\":false}}}]}" } } }After:
{ "timestamp": "2019-06-19T18:53:40.965+0530", "level": "info", "type": "http-log", "detail": { "status": 400, "query_hash": "a0818e9c99f276c36346affe4d4122a16c864e2e", "http_version": "HTTP/1.1", "query_execution_time": 0.001481765, "request_id": null, "url": "/v1/query", "ip": "127.0.0.1", "response_size": 482, "user": { "x-hasura-role": "admin" }, "method": "POST", "detail": { "error": { "internal": { "message": "ConnectionFailure Network.Socket.connect: <socket: 18>: does not exist (Connection refused)", "request": { "proxy": null, "secure": "False", "path": "/graphql", "responseTimeout": "ResponseTimeoutDefault", "method": "POST", "host": "localhost", "requestVersion": "HTTP/1.1", "redirectCount": "10", "port": "8181" } }, "path": "$.args[0].args", "error": "HTTP exception occurred while sending the request to http://localhost:8181/graphql", "code": "remote-schema-error" }, "request": "{\"type\":\"bulk\",\"args\":[{\"type\":\"add_remote_schema\",\"args\":{\"name\":\"random\",\"definition\":{\"url\":\"http://localhost:8181/graphql\",\"headers\":[],\"forward_client_headers\":false}}}]}" } } }Limitation
While adding multiple remote schemas in abulkquery, the error lacks the context of the remote schema which caused merge conflicts, if any, since stitching of all remote schemas happens at the end of the query.Updates after discussion with @0x777 (3rd July 2019):
Do not build GraphQL context at the end of the
bulkquery. Fallback to the current method. However, for each query inbulkquery, the GraphQL context is built but without fetching schemas from remote servers as we're caching them. We should preserve the semantics ofbulkquery ie. treat each query as isolated. Hence, GraphQL schema needs to be built for each bulk query.