knative / eventing

Event-driven application platform for Kubernetes
https://knative.dev/docs/eventing
Apache License 2.0
1.41k stars 588 forks source link

Rename SinkBinding? #2499

Closed nachocano closed 4 years ago

nachocano commented 4 years ago

Problem In 0.12 we introduced SinkBindings, and together with ApiServerSource and PingSource form the core eventing sources. IMO we should try to maintain consistency in how we name sources (especially the core ones), so that is crystal clear, even by just looking at its Kind, that is a source. XSource kind of naming seems to have worked well, and we are using it mostly everywhere. We've been doing something similar for Channels (XChannel). Other disadvantage I see about SinkBinding is that its name is explicitly talking about Sinks, not Sources. IMO the word Binding is more of an implementation detail that shouldn't matter to the folks using this. I think the Subject of such Binding is the piece that should drive the naming. For example:

PodSpecableSource

Persona: Which persona is this feature for? Developer

Exit Criteria Decide on the name for SinkBinding

Time Estimate (optional): Infinite

Additional context (optional) /nitpicking /cc @n3wscott @vaikas @matzew @lionelvillard etc

n3wscott commented 4 years ago

Personally I hate Source in the resource name but folks want it...

n3wscott commented 4 years ago

The thinking with this name is that other bindings around the env are binding this to that. In the case of LogBinding (OSBAPI), it binds a log collector to a subject. SinkBinding binds a resolved sink+sugar for cloudevents into a subject.

It does not bind a source to a subject, the subject is the source implementation. The binding provides the rest of the control plane contract to that implementation to allow it to not need to resolve the sink.

So in this case, I think SinkBinding is the correct direction for this object.

A more verbose name would be something like CloudEventsConsumerBinding and CloudEventsConsumer==Sink in Knative Eventing

mattmoor commented 4 years ago

Source in the resource name is kinda silly if you primarily deal with yaml because it's always right there in the apiVersion:

apiVersion: sources.knative.dev/v1alpha1
kind: Foo

In Go style, this is often referred to as stutter with the package name.

It's also notable that SinkBinding has a wholly different arity than a typical source because it's a meta source. I can use a selector to turn every Deployment in the namespace into a Source, which kinda makes the Deployment the logical source, not the SinkBinding, even though the latter implements the Source duck.

rhuss commented 4 years ago

+1 for being consistent with other sources naming. If SinkBinding IS-A source, then it should follow the (implicit) naming convention for sources, i.e. having a Source suffix in the name.

What is the key-characteristics that all sources have in common ? If we look at the intersection of all sources out there, it's that a source pumps something into sink. The same what a SinkBinding enables (in combination with the subject). However, that something is different from source-to-source (and in case of the SinkBinding its delegated to the subject)

SinkBinding is a binding to/from (let's ignore the direction for a moment) a sink (which actually is the 'source characteristics', i.e. putting something into a sink), so the names Sink and Source a more or less the same in this context imo.

I understand the real source is the combination of the referenced subject + binding part, but what is managed by the Knative user is the binding only (assuming that the referenced subject is managed outside of Knative), which moves the focus on this binding as being the actual managed source.

So long story short: What's about using BindingSource ?

An alternative could be DelegatingSource (or to some degree AdapterSource or ProxySource, but that does not really nail it)

PodSpecableSource would be techically also correct, but maybe its, hmm, too technical ?

rhuss commented 4 years ago

Source in the resource name is kinda silly if you primarily deal with yaml because it's always right there in the apiVersion:

True, but reasoning about objects seldomly involves the API group (expect when dealing with yaml): Documents, clients arguments, kubectl get output,, .. doesn't include the group.

It's also notable that SinkBinding has a wholly different arity than a typical source because it's a meta source

A meta source is a source, too, isn't it ? Technically I totally agree with you, that a SinkBinding is only one half of the equation, but one can equally take the POV, that it's just a super set for a DeploymentSource, JobSource, StatefulSetSource, the same way like a KafkaSource connects a Kafka cluster (the 'real' source) with a sink. IMO a SinkBinding is not much that special.

It was before my time, but I could imagine that the discussion Importer vs. Source went similar :) (aka. Source as name being not technical correct, but still preferred over Importer for some reason)

nachocano commented 4 years ago

the subject is the source implementation

Yes, that's why I think the naming should be driven by the subject... which is the actual source

matzew commented 4 years ago

I like BindingSource - or PodspecableSource

On Tue 4. Feb 2020 at 19:20, Ignacio (Nacho) Cano notifications@github.com wrote:

the subject is the source implementation

Yes, that's why I think the naming should be driven by the subject... which is the actual source

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/knative/eventing/issues/2499?email_source=notifications&email_token=AABGPTTJMFRT4WMREJBAW5LRBGWXRA5CNFSM4KP2445KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKYVE3Q#issuecomment-582046318, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABGPTULOD7L4CQ53NSYWQTRBGWXRANCNFSM4KP2445A .

-- Sent from Gmail Mobile

rhuss commented 4 years ago

the subject is the source implementation

Is the referenced subject as much the source implementation like it is the Kafka cluster for a KafkaSource (with the KafkaSource controller providing the 'binding') ?

In the context of Knative (which added it own interpretation of what a "source" is), I think the naming should be driven by the sink aspect (as this is the common part of all sources).

mattmoor commented 4 years ago

If we look at the intersection of all sources out there, it's that a source pumps something into sink

Except the SinkBinding doesn't pump anything into the sink, it binds that information into the podspecable so that it can.

If SinkBinding IS-A source, then it should follow the (implicit) naming convention for sources, i.e. having a Source suffix in the name

I would say that the SinkBinding IS-A Binding too, perhaps moreso than it is a Source because it's a sort of meta-Source or Source factory. By the logic that these things should both appear in the name, SourceBinding feels most consistent to me.

mattmoor commented 4 years ago

To further clarify my preference for ordering here: What comes first feels like the qualifier on what comes second. It is a Binding, what is it Binding? The Source information into the PodSpecable. Ergo SourceBinding or PodSpecableSourceBinding, though the latter is quite a mouthful. 🤣

{Subject}{Content}Binding feels like the right anatomy of Binding names to me, even if it results in huge names 😬

rhuss commented 4 years ago

Looks like we have some hard time to decide whats being the main noun in a combined word describing two nearly equivalent aspects. As a German I have huge experience in squeezing many words in a single word, so you have to trust me ;-P

Jokes aside, let me bring in another perspective from the client side: The decision we have to make is whether we should offer the binding management on the same level as any other source, or should we use a different syntax for managing bindings (e..g kn source binding .... vs kn binding ...). For eventing source handling will there be any binding other than for binding podspecables ? If we settle on using the same management level for the sink binding as for source (kn source binding create), I really would appreciate a name that follows the other source naming schemes (Source at the end). BTW, currently we already have kn source binding although we are managing "SinkBinding" objects. No problem for us, but confusing for the user.

@n3wscott on slack you mentioned some possible alternate bindings like a TwitterBinding etc., but I have no idea what benefit that would have over a (typed) TwitterSource ? Would the subject part of such a TwitterBinding be any useful outside the context of Knative eventing and used without a binding ? (if its always used together with a binding then it should be combined to a 'classic' source)

rhuss commented 4 years ago

To further clarify my preference for ordering here: What comes first feels like the qualifier on what comes second. It is a Binding, what is it Binding? The Source information into the PodSpecable. Ergo SourceBinding or PodSpecableSourceBinding, though the latter is quite a mouthful.

But the PodSpecable is not a Source, too, as it lacks the sink reference (brought in by the binding). An then why not CloudEvent instead of Source as this might better pinpoint the "content" ? E.g. PodSpecableCloudEventBinding (did I mention that I'm German speaking ? :)

I think we should not strive for absolut technical correctness but for describing the usage best to provide a coherent UX.

My preference so would be still "BindingSource" as a sink binding is been used and managed like a source (in the context of eventing sources). And if it looks like a 🦆, it is a 🦆, isn't it ? :). The alternative could be "MetaSource" but that's probably too vague.

nachocano commented 4 years ago

I think we should not strive for absolut technical correctness but for describing the usage best to provide a coherent UX.

+100

nachocano commented 4 years ago

BindingSource

I'm fine with this. I think is better than SinkBinding

mattmoor commented 4 years ago

And if it looks like a 🦆, it is a 🦆, isn't it ? :).

Binding is a duck too. The spec is literally the blending of two ducks 😂

// SinkBindingSpec holds the desired state of the SinkBinding (from the client).
type SinkBindingSpec struct {
    duckv1.SourceSpec        `json:",inline"`
    duckv1alpha1.BindingSpec `json:",inline"`
}
lionelvillard commented 4 years ago

Let's start with the basics (from the source spec):

A Source is any Kubernetes object that generates or imports an event and relays that event to another endpoint on the cluster via CloudEvents.

The important bit here is relays. And that's exactly what SinkBinding is doing: it relays events produced by a Podspecable to a sink. This is exactly what the GithubSource source is doing, modulo the need for converting events to CloudEvent ones. SinkBinding does not convert (it could?)

As such, I now think BindingSource is a good name, at least the suffix is good.

n3wscott commented 4 years ago

And that's exactly what SinkBinding is doing: it relays events produced by a Podspecable to a sink.

it really is not: Take a deployment case, the source is the combo of the deployment and SinkBinding. The SinkBinding augments the runtime contract of the container in the deployment to allow it to adhere to some of what is required for the impl to understand where to send an event (the sink) and how to argument it (the ceOverrides)

SinkBinding does this runtime augmentation. It also fulfills the control plane aspect of source by allowing a sink to be settable, allowing ceOverrides to be settable, and reporting back the sinkUri of the resolved sink.

So in my mind, the true Source is that Deployment+SinkBinding. Both are fairly useless alone.

This to me looks like:

[Deployment] <-- [SinkBinding] --> [Sink]

a SourceBinding or BindingSource would look like this:

[Source] <-- [SourceBinding] --> [Deployment]

Which is what something like a SlackBinding does:

[Slack] <-- [SlackBinding] --> [Deployment]

Which you can make into a source if you do this:

[Slack] <-- [SlackBinding] --> [Deployment] <-- [SinkBinding] --> [Sink]

For the SlackSource, you need all 5 parts to be deployed if you want to leverage these components and call that thing a Source.

rhuss commented 4 years ago

Which is what something like a SlackBinding does:

[Slack] <-- [SlackBinding] --> [Deployment]

This is the part where I'm struggling a bit. What would be the both ends of such a SlackBinding ?

I'm not seeing yet the benefits of this quite involved approach over a single, typed SlackSource, which would look like

[SlackSource] --> [Sink]

I see the the benefits of bindings to abstract duck types, but not for very specific things like concrete event backends like Slack or Twitter. But it could be only me who has the feeling that we are over-abstracting here.

matzew commented 4 years ago

I'm not seeing yet the benefits of this quite involved approach over a single, typed SlackSource, which would look like

[SlackSource] --> [Sink]

I agree I think the is actually more complicated from users perspective / UX, rather than some good/clean BlahSource CRD, that sinks to some "service"

n3wscott commented 4 years ago

I'm not seeing yet the benefits of this quite involved approach over a single, typed SlackSource, which would look like

[SlackSource] --> [Sink]

This is what you provide ^^

This is what you have to implement in a custom crd:

{Slack} <-- [SlackBinding] --> [Deployment] <-- [SinkBinding] --> {Sink}
n3wscott commented 4 years ago

[SlackBinding] --> [Deployment] <-- [SinkBinding] could be the implementation details that the [SlackSource] does

n3wscott commented 4 years ago

SinkBinding is intended to be a building block, which is why the name is correct with what it does.

grantr commented 4 years ago

SinkBinding is intended to be a building block

This seems different than how SinkBinding has been presented in the past. I don't know about others but my understanding was that SinkBinding was intended to be user API, which is why ContainerSource was argued to be unnecessary.

Do we expect normal users to create SinkBindings? I would love some clarity on the expected use cases.

n3wscott commented 4 years ago

Do we expect normal users to create SinkBindings?

I would expect users to use SinkBinding if they are coding something. And I would expect Contributors to leverage SinkBinding in their source implementation as an internal detail.

ian-mi commented 4 years ago

+1 to a Source-style name.

I believe SinkBinding puts the focus on the wrong place, on what the resource does vs. what it is. This makes it non-obvious that it is meant to be a source without reading the documentation.

SinkBinding provides a contract for creating sources, and that contract so happens to involve a sink binding. However binding a sink is not unique to SinkBinding, every Source must provide a mechanism for binding sinks. Therefore it seems that a generic sounding Source name would be more informative.

Additionally, in future versions of the API we might want to add additional bindings to the runtime contract of SinkBinding, and conversely another context may arise in which a generic resource must provide a mechanism for binding sinks. In either case the name SinkBinding will become confusing.

nachocano commented 4 years ago

We agreed to reach a conclusion by end of this week. Here is the vote summary I compiled from the issue (correct me if I'm wrong):

yeays: @nachocano , @rhuss, @matzew, @lionelvillard, @ian-mi nays: @n3wscott, @mattmoor

Split decision, but it seems that the majority of folks prefer a name consistent with the way we've been using to name Sources (i.e., XSource), considering that we are presenting it to the community as a Source.

The winner is BindingSource, proposed by @rhuss.

If there are no more votes and no more objections, I'll go ahead and start with the renaming. I'll try to have it ready before next Tuesday's cut.

nachocano commented 4 years ago

/assign

lionelvillard commented 4 years ago

Well it's not a Source, this is a component helping source developers to easily write their own source.

@n3wscott Is that the right way of phrasing it?

nachocano commented 4 years ago

Well it's not a Source, this is a component helping source developers to easily write their own source.

@n3wscott Is that the right way of phrasing it?

I agree it's not a Source, so I think we shouldn't advertise it as a "core" Source. For me the core ones are: ApiServerSource PingSource ContainerSource (which IMO shouldn't have been deprecated)

and then we present SinkBindings as a construct to build other powerful sources

n3wscott commented 4 years ago

It is a component that understands the control place runtime contract of Source and applies that to a container.

It being a Source lets things like https://github.com/n3wscott/graph understand that a custom source that a dev writes with a [deployment + sinkbinding] LOOKS like a source, but is not alone. This is the same role as ContainerSource except now we do not limit to just deployments.

n3wscott commented 4 years ago

It is a Source duck. It really is. It is not enough alone to be an event producer, it needs the custom code.

ContainerSource was a nice idea to start and it ends up being too much baggage for us to carry. The amount of feature creep needed to make ContainerSource correctly is too high for eventing to do at the moment with not enough people contributing.

n3wscott commented 4 years ago

Well it's not a Source, this is a component helping source developers to easily write their own source.

@n3wscott Is that the right way of phrasing it?

I think SinkBinding could be moved to a binding.knative.dev group and remain a source ducktype.

It should not be renamed to XXXSource just because it implements that duck.

nachocano commented 4 years ago

I think SinkBinding could be moved to a binding.knative.dev group and remain a source ducktype.

+1 (for me this is another way of saying we are not going to advertise SinkBinding as a core Source)

nachocano commented 4 years ago

It should not be renamed to XXXSource just because it implements that duck.

No, it should not. I think it should be named XXXSource if we are going to present it as a core Source, which it seems that we are finally agreeing that is not :)

n3wscott commented 4 years ago

core

The only thing that matters is Eventing, which includes knative/eventing-contrib.

I do not know what you mean by "advertised" as a Source.

which it seems that we are finally agreeing that is not :)

We do not agree.

nachocano commented 4 years ago

The only thing that matters is Eventing, which includes knative/eventing-contrib.

I see

nachocano commented 4 years ago

I do not know what you mean by "advertised" as a Source.

Say for example to put it as the same level than ApiServerSource and PingSource.

n3wscott commented 4 years ago

It is as useful or maybe more useful than ApiServerSource or PingSource.

nachocano commented 4 years ago

Forgot to close this old issue. No renaming will take place. The previous majority turned around, and most folks didn't find this useful. Closing the issue.