Skip to content
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

feat: allow to set custom timeout for route #4340

Merged
merged 5 commits into from
Jun 1, 2021
Merged

feat: allow to set custom timeout for route #4340

merged 5 commits into from
Jun 1, 2021

Conversation

yxudong
Copy link
Contributor

@yxudong yxudong commented May 30, 2021

What this PR does / why we need it:

Add optional upstream_timeout option for the route.
#4258

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

apisix/balancer.lua Outdated Show resolved Hide resolved
apisix/schema_def.lua Outdated Show resolved Hide resolved
docs/en/latest/admin-api.md Outdated Show resolved Hide resolved
@@ -136,8 +136,18 @@ end
-- set_balancer_opts will be called in balancer phase and before any tries
local function set_balancer_opts(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

We can pass route directly:

function _M.run(route, ctx)

@@ -491,6 +491,15 @@ _M.route = {
minItems = 1,
uniqueItems = true,
},
timeout = {
Copy link
Member

Choose a reason for hiding this comment

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

Better to exact the timeout to a separate schema.




=== TEST 18: valid route with timeout
Copy link
Member

Choose a reason for hiding this comment

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

Need to add test to t/node/timeout-upstream.t

@yxudong yxudong requested a review from spacewander May 31, 2021 03:51
@@ -491,6 +491,15 @@ _M.route = {
minItems = 1,
uniqueItems = true,
},
upstream_timeout = {
Copy link
Contributor

Choose a reason for hiding this comment

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

May abstract the timeout schema so it can be reused by route and upstream schema.

@@ -88,6 +88,7 @@ Note: When the `Admin API` is enabled, it will occupy the API prefixed with `/ap
| script | False | Script | See [Script](architecture-design/script.md) for more | |
| upstream | False | Upstream | Enabled Upstream configuration, see [Upstream](architecture-design/upstream.md) for more | |
| upstream_id | False | Upstream | Enabled upstream id, see [Upstream](architecture-design/upstream.md) for more | |
| upstream_timeout | False | Upstream | Set the upstream timeout for connection, sending and receiving messages of the route. This option will overwrite the [timeout](#upstream) option which set in upstream configuration. | {"connect": 3, "send": 3, "read": 3} |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| upstream_timeout | False | Upstream | Set the upstream timeout for connection, sending and receiving messages of the route. This option will overwrite the [timeout](#upstream) option which set in upstream configuration. | {"connect": 3, "send": 3, "read": 3} |
| upstream_timeout | False | Upstream | Set the upstream timeout for connecting, sending and receiving messages of the route. This option will overwrite the [timeout](#upstream) option which set in upstream configuration. | {"connect": 3, "send": 3, "read": 3} |




=== TEST 18: valid route with upstream_timeout
Copy link
Contributor

Choose a reason for hiding this comment

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

Need further test cases to cover the usage of upstream_timeout.

@yxudong yxudong requested a review from tokers June 1, 2021 03:04
@tokers tokers merged commit 91a048e into apache:master Jun 1, 2021
@yxudong yxudong deleted the yxd branch June 2, 2021 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants