General eventing and kafka updates by abrennan89 · Pull Request #4028 · knative/docs

abrennan89 · 2021-07-17T00:33:09Z

Fixes #3783
Fixes #3078

Proposed Changes

Remove outdated Strimzi install and point to Strimzi quickstart docs instead
Remove yaml sample files in samples/kafka, and references to same, and replace them with including the yaml directly in the doc instead
Remove no-copy from verification command in Eventing installation docs
Replace link to Kafka channel sample with link to knative-sandbox Kafka channel. Not sure on this last one, confirm with eng what's our recommended way to install a Kafka channel. We can maybe remove some of the channel sample docs altogether in this case or replace the YAML samples with what's in sandbox ⚠️ cc @devguyio I'd love your input on this.

knative-prow-robot · 2021-07-17T00:33:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrennan89

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~~ [abrennan89]

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

netlify · 2021-07-17T00:33:15Z

✔️ Deploy Preview for knative ready!

🔨 Explore the source changes: 504226c

🔍 Inspect the deploy log: https://app.netlify.com/sites/knative/deploys/610af415edde5a00089f20f1

😎 Browse the preview: https://deploy-preview-4028--knative.netlify.app/development/developer/eventing/sources

abrennan89 · 2021-07-17T00:47:52Z

/hold

abrennan89 · 2021-07-17T01:15:52Z

If this PR looks like an improvement and doesn't break anything / make anything incorrect, I'd like to get it merged and not hold it up too much on trying to make it perfect. I'd like to move these samples into the main dev/admin guides and just wanted to try to clean it up a bit first.

abrennan89 · 2021-07-17T01:16:53Z

Added a bunch of reviewers but for now I think this just needs and SME review and possibly a lgtm when it's ready.

xtreme-sameer-vohra

Thanks Ashleigh. Added few comments
Disclaimer: I didn't check the auth sections (TLS, etc)

xtreme-sameer-vohra · 2021-07-21T19:26:24Z

+The following example uses a ApiServerSource to publish events to the Broker, and a Trigger that routes events to a Knative Service.
+<!--TODO: Not sure this example makes sense, why would you have an event source AND channels?-->
+
+1. Create a Broker that uses Kafka Channels by default:


There is some missing config as the broker was using InMmemoryChannel and it had an annotation of MTChannelBasedBroker

See Set as default broker implementation > https://knative.dev/v0.23-docs/eventing/broker/kafka-broker/

See above comment - idk if we should even keep this and try to fix it or remove for now

When I was trying this sample a week ago, I actually liked that all of these pieces are in one place, so that I can just follow the steps and see the complete picture without switching in different tabs and getting confused. Even though its not kafka related as you mentioned in your comment above, I think its better to justify with examples here that kafka is actually being used to do these things - just my opinion.

If the broker example stays, I think this is the configmap change that has to go in before the broker is even created as @xtreme-sameer-vohra mentioned, because otherwise it had InMemoryChannel in there which would essentially create imc for all the brokers. But now since we have already changed the default Channel to be a Kafka channel in earlier steps, we can just simply put Kind: Channel

kubectl apply -f - <<EOF --- apiVersion: v1 kind: ConfigMap metadata: name: config-br-default-channel namespace: knative-eventing data: channelTemplateSpec: | apiVersion: messaging.knative.dev/v1 kind: Channel EOF

And related to this, I think you removed both the steps - This will give you two pods, such as: and Inside the Apache Kafka cluster you should see two new topics, such as:. Here I think deleting the pods part makes sense, but about topic creation, it should stay but it should be changed to:

Inside the Apache Kafka cluster you should see one new topic, such as: knative-messaging-kafka.default.default-kne-trigger

I've decided to remove all of the broker stuff from this section, period.
It's too far out of scope to start adding even more ConfigMap details here, particularly when the topic is Kafka Channels, not configuring Brokers.
The plan is to move all of these types of topics to the admin guide anyway, e.g. https://knative.dev/docs/admin/eventing/broker-configuration/, so we'd be duplicating information here to have to clean it up again later.

This was supposed to be a quick PR to do some cleanup, not an entire rewrite of this sample, so I think we should try to get the changes merged so that we have some improvement for now, and we can follow it up with more PRs later to flesh out the full use cases and which other topics need to be included or referenced.

xtreme-sameer-vohra · 2021-07-21T19:31:01Z

Hehe not sure why prow assigned me 😁

snneji · 2021-07-22T17:21:51Z

+The `config-kafka` ConfigMap allows for a variety of Channel options such as:

 - CPU and Memory requests and limits for the dispatcher (and receiver for
-  the distributed channel type) deployments created by the controller


A lot of parentheses in this section. Not sure if they are all necessary.

Probably not, I'd say none of them are, but per my comment above #4028 (comment) I'm not trying to edit this to perfection in this PR - the primary reason for it is to fix the three issues linked, so for things that need additional rewriting like this or the kubectl commands as you mention in the other comment below, I'd prefer if you could raise another issue for us to clean up that stuff later so we're not blocking this PR on it. That OK?

devguyio · 2021-07-23T05:19:51Z

/assign

Co-authored-by: Samia Nneji <snneji@vmware.com>

google-cla · 2021-08-04T19:31:19Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-08-04T19:41:32Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-08-04T19:48:37Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

abrennan89 · 2021-08-04T19:59:51Z

@xinydev good call, removing these since they're basically outdated Strimzi install instructions

google-cla · 2021-08-04T20:00:48Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

abrennan89 · 2021-08-04T20:05:24Z

FYI for reviewers: let's err on the side of removing things that are outdated and doing basic cleanup for now, and then following this PR with other PRs to add more changes. There is a lot of reorg and rewriting that needs to happen with the Kafka content in general - let's not block one PR in the hopes that everything is included at once, please, because it just won't happen.

google-cla · 2021-08-04T20:10:21Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

abrennan89 · 2021-08-04T20:10:49Z

@snneji I think you need to add consent for the cla bot, alternatively @thisisnotapril can you override it please? 🙏🏻

devguyio · 2021-08-05T13:28:38Z

/lgtm

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 17, 2021

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 17, 2021

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 17, 2021

abrennan89 requested review from devguyio and xtreme-sameer-vohra July 17, 2021 00:33

abrennan89 linked an issue Jul 17, 2021 that may be closed by this pull request

Apache Kafka Channel Example(docs/eventing/samples/kafka/channel/index.md) missing step to create service account #3079

Closed

abrennan89 commented Jul 17, 2021

View reviewed changes

Comment thread docs/eventing/samples/kafka/channel/README.md

abrennan89 requested review from RichardJJG, omerbensaadon and snneji July 17, 2021 01:16

xtreme-sameer-vohra suggested changes Jul 21, 2021

View reviewed changes

knative-prow-robot assigned xtreme-sameer-vohra Jul 21, 2021

xtreme-sameer-vohra removed their assignment Jul 21, 2021

snneji suggested changes Jul 22, 2021

View reviewed changes

knative-prow-robot assigned snneji Jul 22, 2021

abrennan89 commented Jul 22, 2021

View reviewed changes

Comment thread docs/eventing/samples/kafka/channel/README.md

abrennan89 commented Jul 22, 2021

View reviewed changes

Comment thread docs/eventing/samples/kafka/channel/README.md

abrennan89 commented Jul 22, 2021

View reviewed changes

Comment thread docs/eventing/samples/kafka/channel/README.md

abrennan89 commented Jul 22, 2021

View reviewed changes

Comment thread docs/eventing/samples/kafka/channel/README.md

knative-prow-robot assigned devguyio Jul 23, 2021

abrennan89 and others added 6 commits August 4, 2021 14:30

Update docs/eventing/samples/kafka/channel/README.md

0ceade3

Co-authored-by: Samia Nneji <snneji@vmware.com>

Update docs/eventing/samples/kafka/channel/README.md

5480845

Co-authored-by: Samia Nneji <snneji@vmware.com>

Update docs/eventing/samples/kafka/channel/README.md

6070a8a

Update docs/eventing/samples/kafka/channel/README.md

4f13441

Update docs/eventing/samples/kafka/channel/README.md

ced10e8

Update docs/eventing/samples/kafka/channel/README.md

2611d81

abrennan89 force-pushed the eventingcleanup branch from d6ce557 to 2611d81 Compare August 4, 2021 19:30

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2021

google-cla Bot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Aug 4, 2021

cleanup rebase

be51b8b

PR comments

6e09211

remove broker fluff, remove old strimzi docs

bfcb02f

abrennan89 requested a review from snneji August 4, 2021 20:06

abrennan89 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2021

fix link

504226c

thisisnotapril added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Aug 4, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2021

knative-prow-robot merged commit 1b25cf8 into knative:mkdocs Aug 5, 2021

abrennan89 deleted the eventingcleanup branch August 26, 2022 17:53

Conversation

abrennan89 commented Jul 17, 2021

Proposed Changes

Uh oh!

knative-prow-robot commented Jul 17, 2021

Uh oh!

netlify Bot commented Jul 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abrennan89 commented Jul 17, 2021

Uh oh!

Uh oh!

abrennan89 commented Jul 17, 2021

Uh oh!

abrennan89 commented Jul 17, 2021

Uh oh!

xtreme-sameer-vohra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xtreme-sameer-vohra Jul 21, 2021

Choose a reason for hiding this comment

Uh oh!

abrennan89 Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

Shashankft9 Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abrennan89 Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

xtreme-sameer-vohra commented Jul 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snneji Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

abrennan89 Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devguyio commented Jul 23, 2021

Uh oh!

google-cla Bot commented Aug 4, 2021

Uh oh!

google-cla Bot commented Aug 4, 2021

Uh oh!

google-cla Bot commented Aug 4, 2021

Uh oh!

abrennan89 commented Aug 4, 2021

Uh oh!

google-cla Bot commented Aug 4, 2021

Uh oh!

abrennan89 commented Aug 4, 2021

Uh oh!

google-cla Bot commented Aug 4, 2021

Uh oh!

abrennan89 commented Aug 4, 2021

Uh oh!

devguyio commented Aug 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

netlify Bot commented Jul 17, 2021 •

edited

Loading

Shashankft9 Aug 4, 2021 •

edited

Loading