这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@tirumaraiselvan
Copy link
Contributor

@tirumaraiselvan tirumaraiselvan commented Sep 24, 2018

Description

Webhooks will be retried if responses contain a "Retry-After" header. This will augment the existing RetryConf settings by retrying even if 1) num_retries have been exhausted , 2) Retry-After is smaller than interval_sec

What component does this PR affect?

  • Server
  • Console
  • CLI
  • Docs
  • Community Content

Requires changes from other components? If yes, please mark the components:

  • Server
  • Console
  • CLI
  • Docs
  • Community Content

Related Issue

Solution and Design

  1. Changes [(HeaderName, HeaderValue)] -> [EventHeaderInfo] in schema cache
  2. Stores request and response headers in invocation logs
  3. Checks for Retry-After header
  4. Changes HTTPStatus error type from HStatus status resp -> HStatus HTTPResp
  5. Some cleanup

Tests

Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have read the contributing guide and my code conforms to the guidelines.
  • This change requires a change in the documentation.
  • I have updated the documentation accordingly.

}
$(deriveToJSON (aesonDrop 4 snakeCase){omitNothingFields=True} ''InvocationRequest)

data InvocationResponse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invocation -> Webhook

getRetryAfterHeaderFromError (HStatus resp) = getRetryAfterHeaderFromResp resp
getRetryAfterHeaderFromError _ = Nothing

getRetryAfterHeaderFromResp resp = let mHeader = find (\(HeaderConf name _) -> CI.mk name == retryAfterHeader) (hrsHeaders resp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

in (name, value)

decodeHeader :: [EventHeaderInfo] -> (N.HeaderName, BS.ByteString) -> HeaderConf
decodeHeader headerInfos (hdrName, hdrVal) = let name = decodeBS $ CI.original hdrName
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

data HeaderValue = HVValue T.Text | HVEnv T.Text
deriving (Show, Eq, Lift)

instance ToJSON HeaderValue where
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This instance should not exist

@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

1 similar comment
@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

@shahidhk
Copy link
Member

@tirumaraiselvan can you add an appropriate PR title and description please?

@shahidhk shahidhk added the c/server Related to server label Sep 26, 2018
@tirumaraiselvan tirumaraiselvan changed the title Retry after Retry event trigger if "Retry-After" header is found in response Sep 26, 2018
@shahidhk shahidhk added the s/do-not-merge Do not merge this pull request to master label Oct 2, 2018
@tirumaraiselvan tirumaraiselvan removed the s/do-not-merge Do not merge this pull request to master label Oct 5, 2018
@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

@tirumaraiselvan
Copy link
Contributor Author

@karthikvt26 Requires console changes to handle new schema of event invocation logs.

@tirumaraiselvan tirumaraiselvan added the s/do-not-merge Do not merge this pull request to master label Oct 17, 2018
@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

@arvi3411301 arvi3411301 added the c/console Related to console label Oct 26, 2018
@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

@hasura-bot
Copy link
Contributor

Review app available at: https://hge-ci-pull-525.herokuapp.com

@shahidhk shahidhk changed the title Retry event trigger if "Retry-After" header is found in response respect retry-after header on event trigger response Oct 26, 2018
@shahidhk shahidhk merged commit baf7c49 into hasura:master Oct 26, 2018
@hasura-bot
Copy link
Contributor

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

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 20, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: aaf9783
Status:⚡️  Build in progress...

View logs

hasura-bot pushed a commit that referenced this pull request May 2, 2024
The massive `Error` enum from `execute.rs` disintegrates into the
following independent error types.
- `RequestError`: All exceptions occurred before executing the root
field plans. Each variant in this error enum contains the error type
stemming from isolated steps in the pipeline involving parsing,
validation, IR conversion, and plan generation.
- `FieldError`: Exception occurred when resolving a field through its
plan. Multiple root fields are executed isolated, and field errors are
collected in a list.

Code paths for `explain` and `execute` query are split into two
functions. This is done to avoid few error variants and unburden the
function that previously does both.

V3_GIT_ORIGIN_REV_ID: 21f2f43ee4805a955fa0ce7d9b45c4b1902def63
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c/console Related to console c/server Related to server s/do-not-merge Do not merge this pull request to master

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants