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.

Sign up for GitHub

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

Jump to bottom

Add single request timeout to the contract #1021

Merged
merged 2 commits into from
Jun 22, 2021
Merged

Add single request timeout to the contract #1021

merged 2 commits into from
Jun 22, 2021

Conversation

slinkydeveloper
Copy link
Contributor

Signed-off-by: Francesco Guardiani francescoguard@gmail.com

This adds the support for the experimental feature delivery-timeout: knative/eventing#5148

Also part of what was discussed in #757

Proposed Changes

  • 🎁 Add EgressConfig.timeout to configure the single request timeout

@slinkydeveloper
Add single request timeout to the contract
3e49634 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 21, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: slinkydeveloper

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow-robot knative-prow-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 21, 2021
@slinkydeveloper
Fix
009acf3 
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@matzew
Copy link
Contributor

matzew commented Jun 21, 2021

Process is:

  1. edit the proto/contract.proto
  2. call the generate-proto.sh script (which updates the two generated files)
  3. manual update the factory here, to make use of the timeout

Right?

@slinkydeveloper
Copy link
Contributor Author

That's exactly what I did for this PR

@matzew
Copy link
Contributor

matzew commented Jun 21, 2021

LGTM

@codecov
Copy link

codecov bot commented Jun 21, 2021 •
edited
Loading

Codecov Report

Merging #1021 (009acf3) into main (5295bd1) will decrease coverage by 0.02%.
The diff coverage is 68.75%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1021      +/-   ##
============================================
- Coverage     75.72%   75.69%   -0.03%     
  Complexity      421      421              
============================================
  Files            74       74              
  Lines          2916     2921       +5     
  Branches        124      127       +3     
============================================
+ Hits           2208     2211       +3     
- Misses          556      557       +1     
- Partials        152      153       +1
Flag Coverage Δ
java-unittests 79.56% <68.75%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...r/dispatcher/http/HttpConsumerVerticleFactory.java 84.76% <68.75%> (-1.24%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5295bd1...009acf3. Read the comment docs.

pierDipi
pierDipi reviewed Jun 21, 2021
View reviewed changes
proto/contract.proto
Comment on lines +50 to +51
// timeout is the single request timeout (not the overall retry timeout)
uint64 timeout = 5;
Copy link
Member

Choose a reason for hiding this comment

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

The controller should set this field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, not in this PR though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to merge delivery-timeout first knative/eventing#5148

Copy link
Member

Choose a reason for hiding this comment

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

Ok :)

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2021
@slinkydeveloper
Copy link
Contributor Author

slinkydeveloper commented Jun 22, 2021 •
edited
Loading

/retest

Tracing headers test flakyness

@knative-prow-robot knative-prow-robot merged commit 900c52b into knative-extensions:main Jun 22, 2021
@slinkydeveloper slinkydeveloper deleted the delivery-timeout branch June 22, 2021 09:37
@slinkydeveloper slinkydeveloper mentioned this pull request Jun 23, 2021
Merged
creydr pushed a commit to creydr/knative-eventing-kafka-broker that referenced this pull request Mar 14, 2024
@matzew
Unpin CE Kafka protocol version from older version (knative-extension…
6bccf12 
…s#1021)

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi left review comments

@matzew matzew matzew left review comments

Assignees

@pierDipi pierDipi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane area/data-plane cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@slinkydeveloper @knative-prow-robot @matzew @pierDipi