Kafka dispatcher no need for a statefulset

[matzew]

The broker itself needs to be a stateful set, but the dispatcher is just a client/consumer.

hence a deployment is good.

@grantr I guess we need some release notes 😄

locally tested w/ a 0.4.0 installation and this branch (and Strimzi 0.11 release). messages are distributed from the dispatcher

Release Note

The `StatefulSet` dispatcher is replaced by a `Deployment`.  and the `StatefulSet` is scaled down to 0 replicas

Action Required: You must delete your old dispatcher, because both dispatchers will dispatch the event

[matzew]

@dubee @nak3 mind to review ?

[dubee]

/lgtm

[knative-prow-robot]

@dubee: changing LGTM is restricted to assignees, and only knative/eventing repo collaborators may be assigned issues.

Details

In response to this:

/lgtm

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.

[evankanderson]

It looks like there is no explicit Service here to address the dispatcher. I'm assuming that is okay?

/lgtm
/approve
/hold

(Remove the hold with /hold cancel assuming that you don't need to add a Service here.)

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dubee, evankanderson, matzew

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:

• OWNERS [evankanderson]

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

[syedriko]

@matzew
If the dispatcher is becoming a Deployment, the README needs to change, too:

[syedriko@localhost config]$ git diff
diff --git a/contrib/kafka/config/README.md b/contrib/kafka/config/README.md
index 5a507cd3..6e28478e 100644
--- a/contrib/kafka/config/README.md
+++ b/contrib/kafka/config/README.md
@@ -80,7 +80,7 @@ kubectl get configmap -n knative-eventing kafka-channel-controller-config
 The Channel Dispatcher receives and distributes all events:
 
```shell
-kubectl get statefulset -n knative-eventing kafka-channel-dispatcher
+kubectl get deployment -n knative-eventing kafka-channel-dispatcher
 ```
 
 The Channel Dispatcher Config Map is used to send information about Channels and

[matzew]

@syedriko ah, right - thanks!

will add this ...

[matzew]

@evankanderson right. I also see no specific need.

I tested it, and also killed it (the pod), while I had a source sinking to a kafka channel, and a subscription for it. Pod got recreated, and eventually the "dumper" service, subscribed to the channel, did continue receive messages.

Perhaps @neosab has some thoughts?

[matzew]

@nak3 mind looking too ?

[nak3]

LGTM 👍

I have tested locally with following env and operations:

• (env) Build from 0.4.0 / Strimzi v0.11.0
• (event source) kubernetes-event-source
• (test) deploy, kill, scaleup/down dispatcher pod, delete/add subscription,

[grantr]

This seems fine to me given my knowledge of the behavior and guarantees of Deployments and Kafka consumers, but I'd like @neosab to weigh in on whether there was any reason to choose a stateful set in the first place.

[evankanderson]

/lgtm
When you are ready, you will need to /hold cancel to get Prow to work on this.

[n3wscott]

Can you change the title to include Kafka?

[matzew]

sure thing

On Wed 3. Apr 2019 at 17:41, Scott Nichols ***@***.***> wrote: Can you change the title to include Kafka? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#972 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJnzvRSglnSS5ZqnKYlv5yV_s-uSVtaks5vdMu_gaJpZM4cJwaA> .

-- Sent from Gmail Mobile

[matzew]

/hold cancel

[akashrv]

In case of an upgrade from 0.5 to 0.6 I noticed that the old kafka dispatcher stays alive and now we end up with 2 dispatchers. The old dispatcher should be deleted or some notes must be added to delete it

[grantr]

@matzew can you add a release note section to this PR with the words Action Required and an explanation that the old dispatcher must be deleted?

I'd also like to know what happens when both dispatchers exist. Do subscriptions get double deliveries?

[akashrv]

@matzew can you add a release note section to this PR with the words Action Required and an explanation that the old dispatcher must be deleted?

I'd also like to know what happens when both dispatchers exist. Do subscriptions get double deliveries?

From what I observed both dispatchers will dispatch the event because the corresponding service selector and the pod labels are the same.

[grantr]

Ok, that needs to be very clearly stated in the release notes then.