Replace Channelable interface with a dedicated CRD #5
Conversation
- add ChannelSubscriptionSet CRD - remove Channelable interface - define that a ChannelSubscriptionSet is created for a Channel by the Provisioner and referenced in the ChannelStatus - update definition of Subscribeable Fixes knative#512
docs/spec/interfaces.md
Outdated
| _Channelable_ resource; the field MAY refer back to this _Subscribable_ as a | ||
| _status.subscriptions_ field (an _ObjectReference_). The resource | ||
| referenced in the _status.subscriptions_ field MUST be a | ||
| _ChannelSubscriptionSet_ resource; the field MAY refer back to this _Subscribable_ as a |
There was a problem hiding this comment.
I'm confused about the self-referencing aspect. Is ChannelSubscriptionSet also Subscribable?
There was a problem hiding this comment.
Yea the self referencing bit is confusing, that's a holdover that I should have removed.
I also proposed that the Subscribable interface be removed in #4, but I'll update this PR to make more sense individually.
docs/spec/spec.md
Outdated
| ### group: eventing.internal.knative.dev/v1alpha1 | ||
|
|
||
| _A ChannelSubscriptionSet holds an aggregation of resolved subscriptions for a | ||
| Channel._ |
There was a problem hiding this comment.
A couple unknowns I'd like to see specified:
- What is responsible for creating this object? Subscription controller, channel provisioner, something else?
- When is it created? Channel creation time or subscription creation time?
There was a problem hiding this comment.
I thought I had a bit saying the ChannelSubscriptionSet should be created by the Provisioner as part of provisioning the channel, but I don't see it. Will add.
docs/spec/spec.md
Outdated
|
|
||
| | Field | Type | Description | Limitations | | ||
| | ------------- | ----------------------- | --------------------------------------------------------------------- | -------------------------------------- | | ||
| | subscribers\* | ChannelSubscriberSpec[] | Information about subscriptions used to implement message forwarding. | Filled out by Subscription Controller. | |
There was a problem hiding this comment.
To clarify what Subscription controller actually does:
- Subscription controller reconciles a Subscription.
- If the Subscription names a Channel in its
from, Subscription controller gets that Channel then gets the ChannelSubscriptionSet in its Status. - Subscription controller looks for a subscriber in ChannelSubscriptionSet matching the Subscription being reconciled, and adds one if none is found. (There's an issue here around losing a reference to the original subscription, so Subscription updates don't work and duplicate subscriptions aren't possible, but that's an issue with the current spec also.)
Is this correct?
There was a problem hiding this comment.
I lifted this line from the current Channelable definition, the only change was to make the field required.
What you listed is the intended behavior. I thought I had also stated this explicitly. I'll add a clarification.
- renamed ChannelSubscriptionSet to ResolvedSubscriptionSet - renamed ChannelSubscriptionSpec to ResolvedSubscription - added details about the Subscription reconciliation process
|
@grantr PTAL |
|
I think we will hold on this one. |
|
Will create a new PR based on all of the other movement. |
Refactoring to new folder structure
Provisioner and referenced in the ChannelStatus
Fixes knative#512
This PR conflicts with #4, but is authored to allow either to merge merged independently. The second to merge will need to be rebased.