-
Notifications
You must be signed in to change notification settings - Fork 67
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
Adds timeout to dispatcher. #687
Adds timeout to dispatcher. #687
Conversation
@@ -42,6 +42,7 @@ func display(event cloudevents.Event) { | |||
} | |||
|
|||
func main() { | |||
flag.Parse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this change is unrelated to this PR. Just something I saw was missing while testing out changes.
Codecov Report
@@ Coverage Diff @@
## main #687 +/- ##
==========================================
- Coverage 76.02% 74.67% -1.35%
==========================================
Files 32 35 +3
Lines 2419 2492 +73
==========================================
+ Hits 1839 1861 +22
- Misses 511 562 +51
Partials 69 69
Continue to review full report at Codecov.
|
/retest |
/hold got some temporary commits on here |
2c4519a
to
35e4b6f
Compare
@gab-satchi we discussed this feature in yesterday's Eventing WG, and because the Kafka folks already have this implemented, they are thinking to promote the feature out of experimental and even pushing some changes to the spec (I haven't reviewed that yet). I believe this is a great way to start engaging with upstream, so we can follow the change all the way up to the spec. |
I think it will be great to add this to the upstream spec. It may make sense to have this default timeout in place as a stopgap to prevent connections being held forever at the dispatcher. Once the eventing core work is done, we can modify this to check there for a timeout and fallback to the default. |
There is a PR to add this to the spec: knative/specs#76. |
/hold cancel |
Is this ready to review cc @gab-satchi ? |
yes it's ready @gabo1208. the 30s timeout will be stopgap while we work on making it configurable in the spec |
/hold will update to use the experimental feature in deliverySpec |
56be470
to
057d290
Compare
/retest |
/retest |
- hardcoded timeout of 30s - modified tests
…ature is turned on
057d290
to
efd60ee
Compare
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gab-satchi, gabo1208 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 |
Changes
NOTE: this change adds about 30s to our unit tests as the timeout is currently not configurable. It didn't make sense to make it configurable just for the tests.
If we choose to make it configurable to the user, we could use the same mechanism to start the dispatcher with lower timeouts for testing.
/kind enhancement
Fixes #671