Adds timeout to dispatcher. #687
Conversation
knative-prow
bot
added
kind/enhancement
size/L
| @@ -42,6 +42,7 @@ func display(event cloudevents.Event) { | |||
| } | |||
|
|
|||
| func main() { | |||
| flag.Parse() | |||
There was a problem hiding this comment.
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 |
knative-prow
bot
added
the
do-not-merge/hold
gab-satchi
force-pushed
the
server-timeouts
branch
from
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 |
knative-prow
bot
removed
the
do-not-merge/hold
|
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 |
knative-prow
bot
added
the
do-not-merge/hold
gab-satchi
force-pushed
the
server-timeouts
branch
3 times, most recently
from
56be470
to
057d290
Compare
|
/retest |
knative-prow
bot
removed
the
do-not-merge/hold
|
/retest |
- hardcoded timeout of 30s - modified tests
…ature is turned on
gab-satchi
force-pushed
the
server-timeouts
branch
from
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