Decide Sink duck type shape

[pierDipi]

Problem
We need to decide the Sink duck type shape.

Persona:
Which persona is this feature for?

• Event Consumer
• System Integrator

Exit Criteria
Sink duck type shape and related documentation.

Additional context (optional)

• @lionelvillard comment: KafkaSink CRD, types and reconciler knative-extensions/eventing-kafka-broker#134 (comment)

  I'm not aware of any discussions around a Sink duck type but I'm sure @n3wscott has some ideas. As you said it must be addressable, and it should includes ceOverrides, contentMode and possibly dataContentType (For RedisStreamSink data can either be json-encoded or RESP-encoded). Anyway do you mind opening an issue?

/help
/area api
/proposal 0.18

cc @knative/eventing-wg-leads @knative/channel-wg-leads @knative/source-wg-leads

[pierDipi]

/remove-help

[n3wscott]

Yep, we need to make up a design doc.

[aslom]

There should be some way to distinguish between different Sinks that only consume events (Addressable) and those that may return events (Callable)? The sink that has response? And also what content types must be supported by data plane - it is now described in details for sinks, channels and brokers linked from
https://github.com/knative/eventing/blob/master/docs/spec/data-plane.md

[pierDipi]

I'm not sure a Sink as I think about it can be Callable, we need probably another name (?) to express this concept.

Also, adding a contentMode in the duck type means that all our sink must support both binary and structured mode, and it's not always the case, or even if we add the MAY keyword to one content mode, users need to know the underlying implementation, so pushing the having or non-having the contentMode field down to the implementation seems more convenient. (I missed the WG meeting yesterday)

[lionelvillard]

The CloudEvent spec says:

Every compliant implementation SHOULD support the structured and binary modes.

At some point we made the decision to support both.

This makes sense because:

Every Source SHOULD support sending events via Binary Content Mode or Structured Content Mode of the HTTP Protocol Binding for CloudEvents.

If Sinks support only one of the two modes, then there is no guarantee of interoperability, which is bad.

Bottom line: sinks must support both modes. IMO having a contentMode field makes no sense to add it to the spec.

I might also misunderstood @slinkydeveloper's comment. "to let the user choose between structured and binary" on the ingress or when sending the event to Kafka itself? I'm guessing you were talking about the latter, right?

[slinkydeveloper]

If Sinks support only one of the two modes,
I might also misunderstood @slinkydeveloper's comment. "to let the user choose between structured and binary" on the ingress or when sending the event to Kafka itself? I'm guessing you were talking about the latter, right?

Yeah I think i wrongly explained myself: of course, on the ingress side of the sink, you should support both binary and structured.

But downstream (the sink egress) could not have support for both (just think about natss that supports only structured), or even better, it could not even support cloudevents. For example, think about an eventual github sink we may design: when it gets events of type comment, it writes the data field as a comment on a specific pr. In that sink example, what contentMode means? Or, to bring another example, in kafka sink we could have another content mode that just takes the payload and writes it in the usual kafka way. So that's something very specific to what's the downstream system IMO. So for me we should not abstract on top of contentMode, because it may not make sense for each sink and because some sinks may extend it in different ways.

[slinkydeveloper]

In the case of KafkaSink we ended up implementing only Addressable, since it's the most appropriate ducktype for a *Sink.

As discussed in the 2020-09-01 Eventing Source meeting, we probably don't need atm any particular Sink ducktype because Addressable already address our needs. For me we can close this issue and reopen in future if we need it

[lionelvillard]

sounds good to me.

[pierDipi]

Sounds good, let's collect users' inputs first.

/close

[knative-prow-robot]

@pierDipi: Closing this issue.

In response to this:

Sounds good, let's collect users' inputs first.

/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.