Make in-memory-channel dispatcher non-blocking

[scothis]

Refs #535

The in-memory-channel currently holds the request open when receiving a new event until the event has been dispatched. If an error is encountered while dispatching, that error is reported back to the caller. This is in contrast to other channels which acknowledge receiving an event to the caller independent of dispatching the event to subscribers.

This behavior is leaking out of the fanout sidecar which when operating as a dispatching sidecar should be blocking. However, the in-memory-channel dispatcher is only the fanout sidecar. To my knowledge, the fanout sidecar is unused other than the in-memory-channel. Rather than scrap the sidecar, I made it optionally non-blocking.

Proposed Changes

• allow the fanout sidecar to optionally be non-blocking, by default it is still blocking
• configure the fanout sidecar in the in-memory-channel dispatcher to be non-blocking

Release Note

The in-memory-channel no longer blocks requests while the event is dispatched. New events will be immediately acked with a 202 Accepted when received. Dispatch errors will not be reported to the caller.

/assign @adamharwayne

[knative-prow-robot]

@scothis: GitHub didn't allow me to assign the following users: adamharwayne.

Note that only knative members and repo collaborators can be assigned.
For more information please see the contributor guide

Details

In response to this:

Refs #535

The in-memory-channel currently holds the request open when receiving a new event until the event has been dispatched. If an error is encountered while dispatching, that error is reported back to the caller. This is in contrast to other channels which acknowledge receiving an event to the caller independent of dispatching the event to subscribers.

This behavior is leaking out of the fanout sidecar which when operating as a dispatching sidecar should be blocking. However, the in-memory-channel dispatcher is only the fanout sidecar. To my knowledge, the fanout sidecar is unused other than the in-memory-channel. Rather than scrap the sidecar, I made it optionally non-blocking.

Proposed Changes

• allow the fanout sidecar to optionally be non-blocking, by default it is still blocking
• configure the fanout sidecar in the in-memory-channel dispatcher to be non-blocking

Release Note

The in-memory-channel no longer blocks requests while the event is dispatched. New events will be immediately acked with a 202 Accepted when received. Dispatch errors will not be reported to the caller.

/assign @adamharwayne

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: scothis

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [scothis]

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

[scothis]

/assign @Harwayne

[Harwayne]

This is similar functionality to what #708 is trying to add. The main difference being that #708 is changing the functionality on a new in-memory CCP and leaving the exiting in-memory-channel CCP synchronous (with the plan to remove it entirely in the following release). The async behavior configurable on a per Channel basis, rather than per binary. My preference is for that one, would that be acceptable @scothis?

/cc @sbezverk
This is a similar PR to #708.

[scothis]

I missed #708, ships crossing in the night...

The async behavior configurable on a per Channel basis, rather than per binary. My preference is for that one, would that be acceptable?

I'm not sure which one you're asking about, but I don't think a blocking channel is useful and in fact is harmful, never mind inefficient. Any channel implementation backed by messaging middleware will inherently decouple receiving an event from dispatching the event.

A common source of bugs is when the semantics of an interface shift slightly depending on configuration external to the caller. Making each channel optionally blocking makes the ConfigMap more complex while only being meaningful in a very narrow use case. Channel arguments should be avoided unless absolutely necessary, particularly so for the default channel provider.

[Harwayne]

Making each channel optionally blocking makes the ConfigMap more complex while only being meaningful in a very narrow use case. Channel arguments should be avoided unless absolutely necessary, particularly so for the default channel provider.

The idea in #708 is not that the user can provide arguments to the Channel. Rather the struct stored in the ConfigMap has an additional asyncHandler boolean for each entry. It is set to true when the user uses the new in-memory CCP and false when they use the old in-memory-channel. Neither CCP supports any arguments. This does make the ConfigMap more complex, but only slightly. A desirable property (in my opinion) is that this allows us to leave the existing synchronous behavior for in-memory-channel, in case someone is relying on it.

I don't think a blocking channel is useful

I now agree. We plan to remove the in-memory-channel in the release after #708 gets in. This gives anyone who is relying on the synchronous behavior a clear signal it will no longer work and one full release to migrate. Those users can continue to use the synchronous in-memory-channel and migrate over to the async in-memory when it is convenient for them.

[sbezverk]

@scothis in general imho having additional option is always better, especially for the debugging, particularly with this PR, it does not seem to exercise this new behaviour. If we go with your PR, then could you please change e2e tests so both blocking and non-blocking path would be tested. In my PR I do see some strangeness happening when a non-blocking fanout handler is called.

[scothis]

/close

[knative-prow-robot]

@scothis: Closing this PR.

Details

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.