Skip to content

feat(translator): implement idle timeout in ClientTrafficPolicy#3056

Merged
guydc merged 8 commits intoenvoyproxy:mainfrom
yaelSchechter:idle-timeout-impl
Apr 2, 2024
Merged

feat(translator): implement idle timeout in ClientTrafficPolicy#3056
guydc merged 8 commits intoenvoyproxy:mainfrom
yaelSchechter:idle-timeout-impl

Conversation

@yaelSchechter
Copy link
Copy Markdown
Contributor

What type of PR is this?
Implement idle timeout in ClientTrafficPolicy
Follow up of #3042

Fixes #2861

Signed-off-by: Yael Shechter <yael.shechter@sap.com>
@yaelSchechter yaelSchechter requested a review from a team as a code owner March 29, 2024 06:27
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2024

Codecov Report

Attention: Patch coverage is 69.23077% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 58.09%. Comparing base (5a66927) to head (1fb625f).

Files Patch % Lines
internal/ir/zz_generated.deepcopy.go 0.00% 5 Missing ⚠️
internal/gatewayapi/clienttrafficpolicy.go 78.57% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3056      +/-   ##
==========================================
+ Coverage   58.08%   58.09%   +0.01%     
==========================================
  Files         165      165              
  Lines       27460    27477      +17     
==========================================
+ Hits        15950    15964      +14     
- Misses      10557    10560       +3     
  Partials      953      953              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mgr.RequestTimeout = durationpb.New(irListener.Timeout.HTTP.RequestReceivedTimeout.Duration)
}

if irListener.Timeout != nil && irListener.Timeout.HTTP != nil && irListener.Timeout.HTTP.IdleTimeout != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is irListener.Timeout != nil && irListener.Timeout.HTTP != nil redundant here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

indeed, removed thanks


case httpIR.Timeout.HTTP == nil:
httpIR.Timeout.HTTP = &ir.HTTPClientTimeout{}
setHTTPTimeout(httpIR)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we initialize this IR container once if it's needed and avoid the init-if-nil call for each member?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

changed it a bit, without this function

Signed-off-by: Yael Shechter <yael.shechter@sap.com>
@yaelSchechter
Copy link
Copy Markdown
Contributor Author

/retest

Copy link
Copy Markdown
Member

@zirain zirain left a comment

Choose a reason for hiding this comment

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

LGTM, docs and e2e tests should be added with following PRs.

@zirain
Copy link
Copy Markdown
Member

zirain commented Apr 2, 2024

/retest

@guydc guydc merged commit 91c18bc into envoyproxy:main Apr 2, 2024
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.

feat: add support to configuring downstream idle timeout

3 participants