KafkaSink CRD, types and reconciler #134

pierDipi · 2020-08-31T08:50:55Z

We can reuse the same receiver component of the data plane to build a KafkaSink.

Proposed Changes

Add KafkaSink CRD, types and reconciler

slinkydeveloper · 2020-08-31T09:14:58Z

So this PR still doesn't contain the KafkaSink controller right? Can you implement the controller in this PR (or even a separate one which depends on this one), i wonder how big the code is 😄

pierDipi · 2020-08-31T09:21:09Z

@slinkydeveloper I would like to consolidate the CRD in this one and then in a separate PR implement the reconciler. WDYT?

pierDipi · 2020-08-31T09:28:46Z

It will probably be roughly 3k+ LOC (validation, types, and reconciler + tests)

slinkydeveloper · 2020-08-31T09:32:16Z

@slinkydeveloper I would like to consolidate the CRD in this one and then in a separate PR implement the reconciler. WDYT?

I'm ok with that, but I think you should start implementing the controller now and keep the 2 PRs separate, this should simplify the feedback on this PR too

slinkydeveloper · 2020-08-31T09:33:40Z

control-plane/config/sink/100-kafka-sink.yaml

+                - numPartitions
+                - replicationFactor
+                - bootstrapServers
+              properties:


I think we need an optional "encoding" flag here, to let the user choose between structured and binary. Also, i think the default should be structured (it's quite unnatural in kafka world to send stuff in headers, so i think kafka users would prefer the structured mode as default)

CE spec calls them message mode: https://github.com/cloudevents/spec/blob/master/spec.md#message, do you think encoding is more descriptive?

To me, on this context, mode seems too "broad"... But i would love to hear people other people opinions @matzew

/cc @lionelvillard @n3wscott

Or contentMode

@slinkydeveloper Is it ok for you if we use contentMode?

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?

knative/eventing#3973

As yesterday discussion during eventing sources wg, I think for now we should proceed forward to shape this KafkaSink CRD as an Addressable and nothing else, since my understanding is that this is the only need for a Sink.
If at some point, we need to abstract something more than Addressable for a sink, we can create that sink duck type and bump this KafkaSink CRD. This process should also help us to better understand what we need to abstract, other than Addressable itself.
WDYT @n3wscott @lionelvillard @pierDipi ?

I would not call it a Sink. The concept is a sink, just like a source is a source in concept. Call it what it is for Kafka is my recommendation, but I have had a lot of pushback on this idea. Folks just want to add the label to everything....

A kafka source would be KafkaSubscription
A kafka sink would be a KafkaTopic

A kafka channel would make n KafkaSubscription for the controlled KafkaTopic

pierDipi · 2020-08-31T09:38:39Z

I'm ok with that, but I think you should start implementing the controller now and keep the 2 PRs separate, this should simplify the feedback on this PR too

Sure

control-plane/config/sink/100-kafka-sink.yaml

pierDipi · 2020-08-31T17:58:46Z

Added types (and code generation): b671be1

control-plane/pkg/reconciler/base/broker/broker_base.go

control-plane/pkg/reconciler/base/broker/condition_set.go

control-plane/pkg/reconciler/base/broker/config.go

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

pierDipi · 2020-09-01T11:05:22Z

Maven...
/retest

pierDipi · 2020-09-01T13:12:23Z

/lint

knative-prow-robot

@pierDipi: 34 warnings.

In response to this:

/lint

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.

control-plane/pkg/reconciler/sink/kafka_sink.go

control-plane/pkg/reconciler/base/broker/broker_base.go

control-plane/pkg/apis/eventing/register.go

control-plane/pkg/reconciler/broker/topic.go

control-plane/pkg/reconciler/trigger/controller.go

control-plane/pkg/reconciler/broker/broker.go

pierDipi · 2020-09-09T14:37:05Z

Rebased.

knative-metrics-robot · 2020-09-09T14:39:28Z

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-eventing-kafka-broker-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
control-plane/pkg/apis/eventing/v1alpha1/kafka_sink_conversion.go	Do not exist	100.0%
control-plane/pkg/apis/eventing/v1alpha1/kafka_sink_defaults.go	Do not exist	100.0%
control-plane/pkg/apis/eventing/v1alpha1/kafka_sink_lifecycle.go	Do not exist	85.7%
control-plane/pkg/apis/eventing/v1alpha1/kafka_sink_types.go	Do not exist	66.7%
control-plane/pkg/apis/eventing/v1alpha1/kafka_sink_validation.go	Do not exist	100.0%
control-plane/pkg/apis/eventing/v1alpha1/register.go	Do not exist	100.0%
control-plane/pkg/config/env_config.go	Do not exist	80.0%
control-plane/pkg/core/config/broker.go	Do not exist	100.0%
control-plane/pkg/core/config/trigger.go	Do not exist	100.0%
control-plane/pkg/core/config/utils.go	Do not exist	75.0%
control-plane/pkg/receiver/path.go	Do not exist	100.0%
control-plane/pkg/reconciler/broker/broker.go	90.5%	89.8%	-0.6
control-plane/pkg/reconciler/broker/broker_config.go	92.3%	91.7%	-0.6
control-plane/pkg/reconciler/kafka/admin.go	Do not exist	87.5%
control-plane/pkg/reconciler/kafka/topic.go	Do not exist	88.5%
control-plane/pkg/reconciler/sink/controller.go	Do not exist	88.9%
control-plane/pkg/reconciler/sink/kafka_sink.go	Do not exist	76.3%
control-plane/pkg/reconciler/trigger/trigger.go	85.7%	85.1%	-0.6

slinkydeveloper · 2020-09-09T14:42:11Z

Are you ok with that?

@pierDipi I think the control plane is fine as is, let's avoid creating too much controller images

pierDipi · 2020-09-09T14:48:26Z

@pierDipi I think the control plane is fine as is, let's avoid creating too much controller images

Just to clarify, I meant to create another YAML, eg

control plane (eventing-kafka-control-plane.yaml) - required
sink data plane (eventing-kafka-sink.yaml)
broker data plane (eventing-kafka-broker.yaml)

Users choose 2 or 3 or both.

Images remain 3 (but we package one per YAML).

slinkydeveloper · 2020-09-09T15:03:09Z

Ah ok so we keep the control plane all together but we split the data plane deployments, one yaml for sink and one for broker. What happens if i install the control plane but only one of the two data planes?

pierDipi · 2020-09-09T15:12:17Z

As it is now you can create a Broker or a KafkaSink but there won't be a data plane for it, so it'll get ready but it isn't actually ready since there is not data plane, we can add a check to see if it exists or not and reflect that in the status.

I would do this in a different PR once this is in.

n3wscott · 2020-09-09T15:11:33Z

control-plane/cmd/kafka-broker-controller/main.go


-	if err := envconfig.Process("", &brokerEnvConfigs); err != nil {
-		log.Fatal("cannot process environment variables", err)
+	brokerEnv, err := config.GetEnvConfig("BROKER")


We should prefix special knative things with K_

I am not sure it is a good idea to call envconfig.Process on each get accessor.

What do you mean by special?

I see those configurations just like SYSTEM_NAMESPACE (which is part of them) which hasn't a K_ prefix. 😕

I am not sure it is a good idea to call envconfig.Process on each get accessor.

How do you set the same set of configurations with different prefixes without duplicating the struct? We have the same set of configurations for two reconcilers with different values, BROKER and SINK are prefixes to distinguish the two: https://github.com/knative-sandbox/eventing-kafka-broker/pull/134/files#diff-2e77fe22a128ec539d95f2e10c96eff6R60-R88,

n3wscott · 2020-09-09T15:17:16Z

control-plane/cmd/webhook-kafka/main.go

+
+var types = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
+
+	eventingv1alpha1.SchemeGroupVersion.WithKind("KafkaSink"): &eventingv1alpha1.KafkaSink{},


I would not call it a Sink. The concept is a sink, just like a source is a source in concept. Call it what it is for Kafka is my recommendation, but I have had a lot of pushback on this idea. Folks just want to add the label to everything....

A kafka source would be KafkaSubscription
A kafka sink would be a KafkaTopic

A kafka channel would make n KafkaSubscription for the controlled KafkaTopic

I suppose the reason why we have the KafkaSource is to keep the naming consistent with the eventing concept event source, similarly to why we should name this KafkaSink to align with the eventing concept of event sink

n3wscott · 2020-09-09T15:18:46Z

control-plane/config/sink/100-kafka-sink.yaml

+                - numPartitions
+                - replicationFactor
+                - bootstrapServers
+              properties:


I would not call it a Sink. The concept is a sink, just like a source is a source in concept. Call it what it is for Kafka is my recommendation, but I have had a lot of pushback on this idea. Folks just want to add the label to everything....

A kafka source would be KafkaSubscription
A kafka sink would be a KafkaTopic

A kafka channel would make n KafkaSubscription for the controlled KafkaTopic

matzew · 2020-09-09T15:48:47Z

I really like this. see a little doc on usage:

https://gist.github.com/matzew/e2c2fcd2696a346f25b8bc9e64bfd0fa

slinkydeveloper · 2020-09-09T15:50:11Z

@matzew cool! We can use your doc as a starting point for the docs pr

n3wscott · 2020-09-09T17:18:33Z

I will relent. Symmetry with the ~~poorly~~ named KakfaSource means KakfaSink 😢

slinkydeveloper

Green flag for me!
/approve
/lgtm
/hold I'm gonna wait for another review

matzew

/lgtm
/approve

knative-prow-robot · 2020-09-10T07:01:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, pierDipi, slinkydeveloper

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [matzew,slinkydeveloper]

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

matzew · 2020-09-10T07:02:29Z

I think the name does match the current "design" - and yes, naming is hard ...

slinkydeveloper · 2020-09-10T07:03:07Z

Who's gonna unhold this? 😄

slinkydeveloper · 2020-09-10T07:05:45Z

/unhold

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2020

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 31, 2020

knative-prow-robot requested review from matzew and slinkydeveloper August 31, 2020 08:51

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 31, 2020

matzew mentioned this pull request Aug 31, 2020

Kafka sink knative/eventing-contrib#1406

Closed

slinkydeveloper reviewed Aug 31, 2020

View reviewed changes

knative-prow-robot requested review from lionelvillard and n3wscott August 31, 2020 09:50

slinkydeveloper reviewed Aug 31, 2020

View reviewed changes

control-plane/config/sink/100-kafka-sink.yaml Outdated Show resolved Hide resolved

pierDipi mentioned this pull request Aug 31, 2020

Decide Sink duck type shape knative/eventing#3973

Closed

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 31, 2020

pierDipi force-pushed the kafka-sink branch 4 times, most recently from d88230b to 835de79 Compare September 1, 2020 10:57

mattmoor reviewed Sep 1, 2020

View reviewed changes

control-plane/pkg/reconciler/base/broker/broker_base.go Outdated Show resolved Hide resolved

control-plane/pkg/reconciler/base/broker/condition_set.go Outdated Show resolved Hide resolved

control-plane/pkg/reconciler/base/broker/config.go Outdated Show resolved Hide resolved

mattmoor reviewed Sep 1, 2020

View reviewed changes

pierDipi force-pushed the kafka-sink branch from 835de79 to 4bb572f Compare September 1, 2020 13:11

knative-prow-robot reviewed Sep 1, 2020

View reviewed changes

pierDipi force-pushed the kafka-sink branch from 4bb572f to 31a6be9 Compare September 1, 2020 13:50

n3wscott reviewed Sep 9, 2020

View reviewed changes

slinkydeveloper approved these changes Sep 10, 2020

View reviewed changes

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2020

knative-prow-robot assigned slinkydeveloper Sep 10, 2020

knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 10, 2020

matzew approved these changes Sep 10, 2020

View reviewed changes

knative-prow-robot assigned matzew Sep 10, 2020

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 10, 2020

knative-prow-robot merged commit d37cf4a into knative-extensions:master Sep 10, 2020

pierDipi mentioned this pull request Sep 10, 2020

Implement KafkaSink finalizer #158

Closed

slinkydeveloper mentioned this pull request Sep 10, 2020

Add Kafka Sink knative/eventing-contrib#682

Closed

This was referenced Sep 10, 2020

Allow independent installation of Broker and KafkaSink #160

Closed

Document Apache Kafka Sink knative/docs#2814

Closed

pierDipi mentioned this pull request Oct 1, 2020

Release notes for 0.18 #240

Closed

pierDipi deleted the kafka-sink branch April 14, 2021 21:18

matzew pushed a commit to matzew/eventing-kafka-broker that referenced this pull request Mar 17, 2022

Slack reporter uses knative-eventing-ci (knative-extensions#134)

b2373dd

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>


		var types = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{

		eventingv1alpha1.SchemeGroupVersion.WithKind("KafkaSink"): &eventingv1alpha1.KafkaSink{},

KafkaSink CRD, types and reconciler #134

KafkaSink CRD, types and reconciler #134

Conversation

pierDipi commented Aug 31, 2020 • edited

Proposed Changes

slinkydeveloper commented Aug 31, 2020 • edited

pierDipi commented Aug 31, 2020

pierDipi commented Aug 31, 2020

slinkydeveloper commented Aug 31, 2020

Choose a reason for hiding this comment

pierDipi Aug 31, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierDipi commented Aug 31, 2020

pierDipi commented Aug 31, 2020 • edited

mattmoor left a comment

Choose a reason for hiding this comment

pierDipi commented Sep 1, 2020

pierDipi commented Sep 1, 2020

knative-prow-robot left a comment

Choose a reason for hiding this comment

pierDipi commented Sep 9, 2020

knative-metrics-robot commented Sep 9, 2020

slinkydeveloper commented Sep 9, 2020

pierDipi commented Sep 9, 2020 • edited

slinkydeveloper commented Sep 9, 2020

pierDipi commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matzew commented Sep 9, 2020

slinkydeveloper commented Sep 9, 2020

n3wscott commented Sep 9, 2020

slinkydeveloper left a comment

Choose a reason for hiding this comment

matzew left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Sep 10, 2020

matzew commented Sep 10, 2020

slinkydeveloper commented Sep 10, 2020

slinkydeveloper commented Sep 10, 2020

pierDipi commented Aug 31, 2020 •

edited

slinkydeveloper commented Aug 31, 2020 •

edited

pierDipi Aug 31, 2020 •

edited

pierDipi commented Aug 31, 2020 •

edited

pierDipi commented Sep 9, 2020 •

edited