Skip to content

router: add dynamic max_stream_duration for upstream #15433

Merged
snowp merged 23 commits intoenvoyproxy:mainfrom
Shikugawa:fix-14260
Sep 3, 2021
Merged

router: add dynamic max_stream_duration for upstream #15433
snowp merged 23 commits intoenvoyproxy:mainfrom
Shikugawa:fix-14260

Conversation

@Shikugawa
Copy link
Copy Markdown
Member

@Shikugawa Shikugawa commented Mar 11, 2021

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: Close #14260. It adds dynamic max_stream_duration to configure for upstream request. It works with setting x-envoy-upstream-stream-timeout-ms and is prior to set toward static upstream max_stream_duration.
Additional Description:
Risk Level: Low
Testing: Unit / Integration
Docs Changes: Required
Release Notes: Required
Fixes #14260

Signed-off-by: Shikugawa <rei@tetrate.io>
@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 15, 2021
@Shikugawa Shikugawa removed the stale stalebot believes this issue/PR has not been touched recently label Apr 16, 2021
@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 16, 2021
@github-actions
Copy link
Copy Markdown

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this May 23, 2021
@Shikugawa Shikugawa reopened this Jul 13, 2021
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa Shikugawa removed the stale stalebot believes this issue/PR has not been touched recently label Jul 14, 2021
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
@Shikugawa Shikugawa requested a review from alyssawilk July 14, 2021 17:03
@Shikugawa
Copy link
Copy Markdown
Member Author

@alyssawilk Could you take a look?

@alyssawilk
Copy link
Copy Markdown
Contributor

mind checking out CI first - looks like there's a tidy failure?
/wait

Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
@alyssawilk
Copy link
Copy Markdown
Contributor

looks like clang tidy may need a look (and unsure if coverage is a flake or not but I'll let you check)
/wait

Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Looking good! A few comments from my end, mostly nits :-)

Signed-off-by: Shikugawa <rei@tetrate.io>
alyssawilk
alyssawilk previously approved these changes Aug 5, 2021
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

LGTM modulo one tiny nit.
@snowp would you be willing to do a pass as well?

Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks! A few comments, overall this makes sense to me

@snowp snowp added the waiting label Aug 9, 2021
Signed-off-by: Shikugawa <rei@tetrate.io>
Signed-off-by: Shikugawa <rei@tetrate.io>
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks, just a few more nits and then I think we're good to go

Signed-off-by: Shikugawa <rei@tetrate.io>
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM, deferring to @alyssawilk for final merge

@alyssawilk
Copy link
Copy Markdown
Contributor

looks good modulo the coverage failure. I'll try a re-run but it may need a main merge
/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15433 (comment) was created by @alyssawilk.

see: more, trace.

@alyssawilk
Copy link
Copy Markdown
Contributor

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15433 (comment) was created by @alyssawilk.

see: more, trace.

@Shikugawa
Copy link
Copy Markdown
Member Author

@alyssawilk Could we merge?

@snowp snowp merged commit d7e1f92 into envoyproxy:main Sep 3, 2021
@Shikugawa Shikugawa deleted the fix-14260 branch September 4, 2021 00:49
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.

Update max_stream_duration dynamically

4 participants