Skip to content

Adding docs/spec shell (v2).#286

Merged
knative-prow-robot merged 13 commits into
knative:masterfrom
n3wscott:spec
Aug 7, 2018
Merged

Adding docs/spec shell (v2).#286
knative-prow-robot merged 13 commits into
knative:masterfrom
n3wscott:spec

Conversation

@n3wscott
Copy link
Copy Markdown
Contributor

replaces #147

@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: n3wscott
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: evankanderson

If they are not already assigned, you can assign the PR to them by writing /assign @evankanderson in a comment when ready.

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:

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

@google-prow-robot google-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 25, 2018
@n3wscott
Copy link
Copy Markdown
Contributor Author

/assign @evankanderson

@@ -0,0 +1,32 @@
// This is the overview graph for object refrences in the control plane.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This graph does not include Flows at all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a Flow, but flow is flexible so it is hard to represent in a single graph

Comment thread docs/spec/README.md Outdated

This directory contains the specification of the Knative Eventing API, which is
implemented in [`channels.knative.dev`](/pkg/apis/channels/v1alpha1) and
[`feeds.knative.dev`](/pkg/apis/feeds/v1alpha1) and verified via [the e2e
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't mention Flows at all.

Comment thread docs/spec/overview.md
@@ -0,0 +1,80 @@
# Resource Groups
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I linked to it. That doc has the wrong abstraction. We did not abstract the bus, we abstracted the channel.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we have the wrong API group names. I could be convinced either way.

Comment thread docs/spec/overview.md Outdated
@@ -0,0 +1,80 @@
# Resource Groups

Knative Eventing API is grouped into _Channels_ and _Feeds_:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No mention of Flows.

Comment thread docs/spec/overview.md Outdated

* _Feeds_ bridge the event source into the eventing framework.

* _Flows_ are the higher order abstractions for eventing that use _Channels_
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to talk about what Flows gives the user, rather than where it sits in relation to other objects.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am at a loss for the correct words for this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the desired path from an external Source of events to a destination that will react to the events

?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not strictly "external", e.g. the k8sevents EventSource

Copy link
Copy Markdown
Contributor Author

@n3wscott n3wscott Jul 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying: * _Flows_ abstract the path of an event from a source takes to reach an event consumer.

Comment thread docs/spec/overview.md Outdated

## EventType

TODO
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we fill this out, with at least some justification?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pointed it to the Architecture page from docs.

Comment thread docs/spec/overview.md Outdated

# Eventing

TODO an overview of how these objects are created and perhaps the persona
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this section in this doc? If so, it seems like the overview should be at the top rather than near the bottom. (I don't quite understand what should be in this section.)

Comment thread docs/spec/README.md Outdated

**Updates to this spec should include a corresponding change to the
API implementation for [channels](/pkg/apis/channels/v1alpha1) or
in [feeds](/pkg/apis/feeds/v1alpha1) and [the e2e test](/test/e2e).**
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be the right place, but it seems like it would be useful to include a section on either "Known issues" with the API and/or patterns that we've agreed upon. In particular, the use of containers/images for extension of Buses and Sources is probably worth highlighting somewhere.

The Parameters/ParametersFrom pattern for passing args to the images is probably also worth documenting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a good TODO but I don't have the context to fully author those. I added a todo for you :D

Copy link
Copy Markdown
Contributor Author

@n3wscott n3wscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, PTAL. Thanks for the review!

Comment thread docs/spec/README.md Outdated

**Updates to this spec should include a corresponding change to the
API implementation for [channels](/pkg/apis/channels/v1alpha1) or
in [feeds](/pkg/apis/feeds/v1alpha1) and [the e2e test](/test/e2e).**
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is a good TODO but I don't have the context to fully author those. I added a todo for you :D

@@ -0,0 +1,32 @@
// This is the overview graph for object refrences in the control plane.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a Flow, but flow is flexible so it is hard to represent in a single graph

Comment thread docs/spec/overview.md
@@ -0,0 +1,80 @@
# Resource Groups
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I linked to it. That doc has the wrong abstraction. We did not abstract the bus, we abstracted the channel.

Comment thread docs/spec/overview.md Outdated

* _Feeds_ bridge the event source into the eventing framework.

* _Flows_ are the higher order abstractions for eventing that use _Channels_
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am at a loss for the correct words for this

Comment thread docs/spec/overview.md Outdated

## EventType

TODO
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pointed it to the Architecture page from docs.

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more comments -- thanks for writing this up and pinging me about actually getting back to the review.

Comment thread docs/spec/README.md Outdated

* [Motivation and goals](motivation.md)
* [Resource type overview](overview.md)
<!-- * [Knative Eventing API spec](spec.md) -->
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark these as TODO?

Comment thread docs/spec/images/overview-reference.dot Outdated
subgraph cluster_B {
Channel
Subscription;
label = "Bus";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure a "Bus" contains a Channel or Subscription in the same way that a Flow "contains" a Feed or Subscription. In the former case, the implementation of the Channel or Subscription is done by the Bus's controller, but in the latter case, the Flow literally describes the spec of the Feed and Subscription objects in some way. It feels more like a "Bus" is referenced by a Channel (and indirectly via Subscription).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the graph to have Channel point to Bus, but had to giveup a little formatting OCD because graphviz does what it wants sometimes.

Comment thread docs/spec/motivation.md Outdated
@@ -0,0 +1,17 @@
# Motivation

Knative Eventing implements the common components for an event delivery
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd turn this around (this talks about what we did, not why):

The goal of Knative Eventing is to define common, composable primitives to enable late-binding event sources and event consumers.

[Why late-binding]

Alternately, you could steal the following principles:

  1. Services are loosely coupled during development and deployed independently on a variety of platforms (Kubernetes, VMs, SaaS or FaaS).
  2. A producer can generate events before a consumer is listening, and a consumer can express an interest in an event or class of events that is not yet being produced.
  3. Services can be connected to create new applications
    a. without modifying producer or consumer.
    b. with the ability to select a specific subset of events from a particular producer

From there, I think it's fine to reference CloudEvents and Kubernetes event-processing or lack thereof.

Comment thread docs/spec/overview.md
The primary resources in the Knative Eventing _Feeds_ API are EventSource,
EventType and Feed:

* **EventSource**, an archetype of an event producer.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem entirely clear; maybe give an example, like github.com or storage.googleapis.com. It might also be worth mentioning whether alternate environments/implementations of the API are separate Sources or a parameter in the Feed. (e.g. do you have different EventSources for each AWS region, or is that a Feed status in some way?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added an example to each. See if that helps.

Comment thread docs/spec/overview.md Outdated

* **EventSource**, an archetype of an event producer.

* **EventType**, the schema for an event.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's worth calling out that there may be multiple EventTypes for a single Source. Again, a worked example might be illuminating, even if it is aspirational rather than factual.

You might also want to say something about EventType implying some sort of more-specific schema for the payload (data) of the event.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should go into the spec part that is still TODO. This is suppose to be the overview.

Comment thread docs/spec/overview.md
The primary resources in the Knative Eventing _Channels_ API are Channel,
Subscription and Bus:

* **Channel**, a named endpoint which accepts and forwards events.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, it might be useful to mention the cardinality here -- a Bus may support many Channels, and a Channel may support multiple Subscriptions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are details of our choice of implementation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think these are reasons for these to be different objects (like Pod, ReplicaSet, and Deployment).

Comment thread docs/spec/overview.md

* **Channel**, a named endpoint which accepts and forwards events.

* **Bus**, an implementation of an event delivery mechanism.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd highlight that a Bus implements some sort of store-and-forward fanout strategy (whether that is Kafka, GCP PubSub, or in-memory).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I think these are details of our choice of implementation?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the role of the Bus is to provide the store-and-forward delivery implementation. Some may do that more durably than others, but a Bus object describes a particular implementation strategy for the store and forward contract.

Comment thread docs/spec/overview.md Outdated

* **Bus**, an implementation of an event delivery mechanism.

* **Subscription**, an expressed interest in events to be delivered to a
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... events from a channel to be delivered to an HTTP endpoint. (Which might be a Knative service, or any other HTTP implementation.)

Comment thread docs/spec/overview.md Outdated
* **Subscription**, an expressed interest in events to be delivered to a
service.

The primary resources in the Knative Eventing _Flows_ API are Flow:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"resource" (no "s")

It might make more sense to say this as The _Flows_ API implements a single resource, Flow:

Comment thread docs/spec/overview.md Outdated

The primary resources in the Knative Eventing _Flows_ API are Flow:

* **Flow**, the connection between an event producer to the event consumer.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you express that this is a high-level intent which abstracts both Flow and Subscription (as well as most of the other resources)?

https://github.com/knative/serving/blob/master/docs/spec/overview.md#service may have some useful verbiage.

Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking better, but I'm still concerned that the object model doesn't clearly justify the choices and objects.

Thanks for starting this; I'm happy to add the remaining docs over time, but would like the initial object model to explain the decomposition of work and value added.

/approve

Comment thread docs/spec/overview.md

* **Channel**, a named endpoint which accepts and forwards events.

* **Bus**, an implementation of an event delivery mechanism.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the role of the Bus is to provide the store-and-forward delivery implementation. Some may do that more durably than others, but a Bus object describes a particular implementation strategy for the store and forward contract.

Comment thread docs/spec/overview.md
* **Subscription**, an expressed interest in events from a _Channel_ to be
delivered to an HTTP endpoint.

An example _Channels_ setup: A _Channel_ has declared it is implemented by a
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the example here doesn't elucidate much. I'd rather see something like "cluster bus X is implemented via NATS, and has 2 channels: 'pulls' and 'analysis'. 'pulls' has one subscription, and 'analysis' has none at the moment.

Comment thread docs/spec/overview.md
the event consumer leveraging _Feed_s and _Channels_.


An example _Flows_ setup: The _Flow_ describes the same properties as the _Feed_
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it would be better to give an example of buffering the GitHub notifications if the target is not up yet (to contrast with Feed, which would not).

Comment thread docs/spec/overview.md
![Object Model](images/overview-reference.png)

See the [Knative Eventing Docs
Architecture](https://github.com/knative/docs/blob/master/eventing/README.md#architecture)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "architecture" mean as a modifier to docs?

@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, n3wscott

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2018
@n3wscott
Copy link
Copy Markdown
Contributor Author

n3wscott commented Aug 7, 2018

@grantr for lgtm?

@grantr
Copy link
Copy Markdown
Contributor

grantr commented Aug 7, 2018

/lgtm
Thanks for writing this @n3wscott. I think it's sufficient to describe the what for now and add the why in future PRs.

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2018
@knative-prow-robot knative-prow-robot merged commit 291c121 into knative:master Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants