add timeout to remote schema calls (close #2501) #2753

tirumaraiselvan · 2019-08-20T09:36:32Z

Description

Adds an optional timeout_seconds argument to add_remote_schema API

Affected components

Server
Console
Docs

Related Issues

#2501

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

netlify · 2019-08-20T09:37:53Z

Deploy preview for hasura-docs ready!

Built with commit d39a04c

https://deploy-preview-2753--hasura-docs.netlify.com

hasura-bot · 2019-08-20T10:03:16Z

Review app for commit 3816f3c deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-3816f3cd

hasura-bot · 2019-08-20T11:45:28Z

Review app for commit 9910b20 deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-9910b203

hasura-bot · 2019-08-21T03:47:39Z

Review app for commit 3e3dde0 deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-3e3dde08

rikinsk-zz · 2019-08-21T12:40:08Z

@tirumaraiselvan Can you update the API changes in docs

hasura-bot · 2019-08-21T14:01:54Z

Review app for commit b48d3d5 deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-b48d3d5a

server/src-lib/Hasura/Server/Utils.hs

server/src-lib/Hasura/RQL/Types/RemoteSchema.hs

lexi-lambda · 2019-08-22T08:59:55Z

server/src-lib/Hasura/HTTP.hs

 wreqOptions manager hdrs =
  Wreq.defaults
-  & Wreq.headers .~  contentType : userAgent : hdrs
+  & Wreq.headers .~  defaultHeaders <> hdrs


Don’t we need to avoid adding userAgent here if it’s in hdrs already? I think it’s legal for certain HTTP headers to be specified multiple times, so I imagine wreq will currently just send both. (It’d be nice to be able to have a test case for this, but IIUC it’s not super easy to do with the current test harness?)

Right! Thanks.

This reverts commit 3e3dde0.

hasura-bot · 2019-08-23T03:49:29Z

Review app for commit 4258210 deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-42582106

tirumaraiselvan · 2019-08-23T04:21:03Z

server/src-lib/Hasura/Events/Lib.hs

-  [ (CI.mk "Content-Type", "application/json")
-  , (CI.mk "User-Agent", "hasura-graphql-engine/" <> T.encodeUtf8 currentVersion)
-  ]
+addDefaultHeaders hdrs = defaultHeaders <> rmDefaultHeaders hdrs


@lexi-lambda This change has led me to believe that we have HTTP clients which are not using the Hasura.HTTP library basically (event trigger module and remote schema module):

The only modules which seem to be using this are:

Telemetry
Auth
Auth/JWT
CheckUpdates

Is it fine for EventTriggers and RemoteSchemas to maintain their own clients because these might be specific to these modules.

This function appears to prioritize default headers over the headers that were passed in, but the code in GraphQL.Execute appears to prioritize the headers that were passed in over the default headers. Is the difference intentional?

Also, can this code at least share the defaultHeaders declaration with the one in Hasura.HTTP? Having it duplicated in both places seems worth avoiding if we can help it.

This was my question. Is it worth sharing these tiny functions, when the bulk of the client is specified per module itself? Shouldn't HTTP.Hasura give out a client with these baked in for standard usage? Those who are not using it may do their own thing including defaultHeaders?

I don’t know, as I haven’t looked at the code in detail, but my gut instinct is that

these modules should probably be using Hasura.HTTP,

but if they aren’t, and you don’t want to do the refactoring to make them use it right now, sharing the default headers is an alright first step.

If the first assumption isn’t true, then you could be right, but I’d be surprised if it were. Is there a reason these modules shouldn’t be using the same library?

I don't think there is any real reason except the library didn't have proper functions to use with Event Triggers. I guess we should make an attempt to consolidate the library then.

Cool, I am just going to share the functions then. Let's do the full consolidation in a separate no-op refactor PR.

Sounds good. 👍

hasura-bot · 2019-08-23T04:25:29Z

Review app for commit cd5e656 deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-cd5e656c

hasura-bot · 2019-08-23T05:00:41Z

Review app for commit 1bbbc46 deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-1bbbc462

hasura-bot · 2019-08-23T05:21:12Z

Review app for commit 1c6285d deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-1c6285d8

hasura-bot · 2019-08-23T08:02:38Z

Review app for commit 0c444f6 deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-0c444f6a

hasura-bot · 2019-08-23T08:16:49Z

Review app for commit d39a04c deployed to Heroku: https://hge-ci-pull-2753.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2753-d39a04ca

hasura-bot · 2019-08-23T08:57:22Z

Review app https://hge-ci-pull-2753.herokuapp.com is deleted

…hasura#2753)

add timeout to remote schema calls

8cc2e51

tirumaraiselvan requested a review from lexi-lambda as a code owner August 20, 2019 09:36

add test

3816f3c

console changes for issue-2501

9910b20

tirumaraiselvan requested a review from rikinsk-zz as a code owner August 20, 2019 11:38

wawhal assigned rikinsk-zz Aug 20, 2019

tirumaraiselvan assigned ecthiender Aug 20, 2019

pass User-Agent header from client

3e3dde0

tirumaraiselvan mentioned this pull request Aug 21, 2019

Remote schema forward original user-agent #2572

Closed

tirumaraiselvan changed the title ~~add timeout to remote schema calls (close #2501)~~ add timeout to remote schema calls and pass client headers to remote server (close #2501, #2572) Aug 21, 2019

update timoout title

b48d3d5

rikinsk-zz previously approved these changes Aug 21, 2019

View reviewed changes

0x777 reviewed Aug 22, 2019

View reviewed changes

server/src-lib/Hasura/Server/Utils.hs Outdated Show resolved Hide resolved

lexi-lambda reviewed Aug 22, 2019

View reviewed changes

server/src-lib/Hasura/RQL/Types/RemoteSchema.hs Show resolved Hide resolved

lexi-lambda reviewed Aug 22, 2019

View reviewed changes

tirumaraiselvan assigned lexi-lambda and unassigned ecthiender Aug 22, 2019

Revert "pass User-Agent header from client"

4258210

This reverts commit 3e3dde0.

tirumaraiselvan dismissed rikinsk-zz’s stale review via 4258210 August 23, 2019 03:41

tirumaraiselvan changed the title ~~add timeout to remote schema calls and pass client headers to remote server (close #2501, #2572)~~ add timeout to remote schema calls (close #2501) Aug 23, 2019

tirumaraiselvan added 2 commits August 23, 2019 09:47

fix duplicate default headers

f970dba

fix duplicate default headers for event triggers

cd5e656

tirumaraiselvan commented Aug 23, 2019

View reviewed changes

use Hasura.HTTP everywhere for default headers

1bbbc46

add field in docs

1c6285d

tirumaraiselvan requested a review from marionschleifer as a code owner August 23, 2019 05:12

lexi-lambda previously approved these changes Aug 23, 2019

View reviewed changes

marionschleifer previously approved these changes Aug 23, 2019

View reviewed changes

rikinsk-zz previously approved these changes Aug 23, 2019

View reviewed changes

Merge branch 'master' into remote-schema-configure-timeout

0c444f6

fix typo

d39a04c

rikinsk-zz dismissed stale reviews from marionschleifer, lexi-lambda, and themself via d39a04c August 23, 2019 08:08

rikinsk-zz approved these changes Aug 23, 2019

View reviewed changes

0x777 merged commit 9878421 into hasura:master Aug 23, 2019

polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020

allow configuring timeout for remote schema calls (close hasura#2501) (…

3501acd

…hasura#2753)

add timeout to remote schema calls (close #2501) #2753

add timeout to remote schema calls (close #2501) #2753

Uh oh!

Conversation

tirumaraiselvan commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Affected components

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Uh oh!

netlify bot commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasura-bot commented Aug 20, 2019

Uh oh!

hasura-bot commented Aug 20, 2019

Uh oh!

hasura-bot commented Aug 21, 2019

Uh oh!

rikinsk-zz commented Aug 21, 2019

Uh oh!

hasura-bot commented Aug 21, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasura-bot commented Aug 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hasura-bot commented Aug 23, 2019

Uh oh!

hasura-bot commented Aug 23, 2019

Uh oh!

hasura-bot commented Aug 23, 2019

Uh oh!

hasura-bot commented Aug 23, 2019

Uh oh!

hasura-bot commented Aug 23, 2019

Uh oh!

hasura-bot commented Aug 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tirumaraiselvan commented Aug 20, 2019 •

edited

Loading

netlify bot commented Aug 20, 2019 •

edited

Loading