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

Glue the new DeliverySpec.Timeout #1034

Merged

Conversation

slinkydeveloper
Copy link
Contributor

@slinkydeveloper slinkydeveloper commented Jun 23, 2021

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

Fixes #757, followup of #1021, part of knative/eventing#5148

Proposed Changes

  • 🎁 Support the new DeliverySpec.Timeout field

Release Note

Now you can specify both in Broker and Trigger delivery specs the new timeout field, as part of the experimental feature delivery-timeout. For more details: https://knative.dev/docs/eventing/experimental-features/ 

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper requested a review from pierDipi June 23, 2021 12:56
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 23, 2021
@knative-prow-robot knative-prow-robot added area/control-plane approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 23, 2021
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

Can we add a reconciler test with this timeout?

/approve

@slinkydeveloper
Copy link
Contributor Author

Can we add a reconciler test with this timeout?

I'm gonna add one upstream and then set it up here

@pierDipi
Copy link
Member

I meant a test for ReconcileKind

@codecov
Copy link

codecov bot commented Jun 23, 2021

Codecov Report

Merging #1034 (5742cc7) into main (4931717) will increase coverage by 0.11%.
The diff coverage is 80.00%.

❗ Current head 5742cc7 differs from pull request most recent head 68dd16a. Consider uploading reports for the commit 68dd16a to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1034      +/-   ##
============================================
+ Coverage     75.10%   75.22%   +0.11%     
- Complexity      420      427       +7     
============================================
  Files            75       77       +2     
  Lines          2928     2938      +10     
  Branches        128      128              
============================================
+ Hits           2199     2210      +11     
+ Misses          570      569       -1     
  Partials        159      159              
Flag Coverage Δ
java-unittests 79.31% <ø> (+0.20%) ⬆️

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

Impacted Files Coverage Δ
control-plane/pkg/core/config/utils.go 75.00% <80.00%> (+0.53%) ⬆️
.../core/reconciler/impl/ResourcesReconcilerImpl.java 92.70% <0.00%> (-0.45%) ⬇️
...enting/kafka/broker/receiver/ReceiverVerticle.java 92.85% <0.00%> (ø)
...ka/broker/dispatcher/ConsumerDeployerVerticle.java 69.09% <0.00%> (ø)
...knative/eventing/kafka/broker/dispatcher/Main.java
...venting/kafka/broker/dispatcher/DispatcherEnv.java
...ka/broker/core/reconciler/ResourcesReconciler.java 100.00% <0.00%> (ø)
...ng/kafka/broker/dispatcher/main/DispatcherEnv.java 0.00% <0.00%> (ø)
...re/reconciler/impl/ResourcesReconcilerBuilder.java 100.00% <0.00%> (ø)
...ve/eventing/kafka/broker/dispatcher/main/Main.java 0.00% <0.00%> (ø)
... and 2 more

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 4931717...68dd16a. Read the comment docs.

Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@slinkydeveloper slinkydeveloper requested a review from pierDipi June 24, 2021 07:21
Signed-off-by: Francesco Guardiani <francescoguard@gmail.com>
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-eventing-kafka-broker-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
control-plane/pkg/core/config/utils.go 88.9% 88.1% -0.8

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

Thanks!

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, 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:
  • OWNERS [pierDipi,slinkydeveloper]

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 merged commit 219a7c0 into knative-extensions:main Jun 24, 2021
@slinkydeveloper slinkydeveloper deleted the delivery_timeout branch June 24, 2021 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support configuration of Dispatcher CircuitBreaker
4 participants