Skip to content

Add Apache Kafka Broker #2740

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 18, 2020
Merged

Add Apache Kafka Broker #2740

merged 1 commit into from
Aug 18, 2020

Conversation

@pierDipi
Copy link
Member

@pierDipi pierDipi commented Aug 11, 2020

  • Add Apache Kafka Broker

/cc @abrennan89 @slinkydeveloper

@knative-prow-robot 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 11, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 11, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 11, 2020
@pierDipi pierDipi force-pushed the add-kafka-broker branch 2 times, most recently from 4328108 to 24ccc79 Compare August 11, 2020 10:34
slinkydeveloper
slinkydeveloper reviewed Aug 11, 2020
View reviewed changes
@pierDipi
Copy link
Member Author

pierDipi commented Aug 11, 2020

Once this is in, I'll follow up with an example to put here: https://knative.dev/development/eventing/samples/kafka/

pierDipi
pierDipi commented Aug 11, 2020
View reviewed changes
@pierDipi
Copy link
Member Author

pierDipi commented Aug 12, 2020

/retest

@pierDipi pierDipi changed the title [WIP] Add Apache Kafka Broker Add Apache Kafka Broker Aug 12, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 12, 2020
@pierDipi pierDipi changed the title Add Apache Kafka Broker [WIP] Add Apache Kafka Broker Aug 12, 2020
@knative-prow-robot 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 12, 2020
@pierDipi
Copy link
Member Author

pierDipi commented Aug 12, 2020

/retest

abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
mpetason
mpetason reviewed Aug 12, 2020
View reviewed changes
abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
docs/eventing/broker/kafka-broker.md Outdated
Comment on lines 7 to 8
This page shows how to install and configure the
[Kafka Broker](https://github.com/knative-sandbox/eventing-kafka-broker).
Copy link
Contributor

@abrennan89 abrennan89 Aug 12, 2020

Choose a reason for hiding this comment

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

What's the user story for this, i.e. why would someone use this over the default Knative broker?
This would probably be a good place for some introductory content, not so much about what the Kafka broker is (we could include the link from earlier here again maybe), but why as a user would you want to install or use this?

Copy link
Member Author

@pierDipi pierDipi Aug 13, 2020

Choose a reason for hiding this comment

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

I added this https://github.com/knative/docs/pull/2740/files#diff-20631ccdd823c2921c4d6bda9b486bfaR7-R17, please, let me know if it helps.

abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
docs/eventing/broker/kafka-broker.md
Comment on lines 33 to 78
## Create a Kafka Broker

A Kafka Broker object looks like this:

```yaml
apiVersion: eventing.knative.dev/v1
kind: Broker
metadata:
annotations:
# case-sensitive
eventing.knative.dev/broker.class: Kafka
name: default
namespace: default
spec:
# Configuration specific to this broker.
config:
apiVersion: v1
kind: ConfigMap
name: kafka-broker-config
namespace: knative-eventing
# Optional dead letter sink, you can specify either:
# - deadLetterSink.ref, which is a reference to a Callable
# - deadLetterSink.uri, which is an absolute URI to a Callable (It can potentially be out of the Kubernetes cluster)
delivery:
deadLetterSink:
ref:
apiVersion: serving.knative.dev/v1
kind: Service
name: dlq-service
```
Copy link
Contributor

@abrennan89 abrennan89 Aug 12, 2020

Choose a reason for hiding this comment

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

Move this to the beginning of the doc instead and use it to explain what's required and why in a kafka broker

Copy link
Member Author

@pierDipi pierDipi Aug 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Above the installation?

abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
docs/eventing/broker/kafka-broker.md
Comment on lines 64 to 99
`spec.config` should reference any `ConfigMap` that looks like the following:

```yaml
apiVersion: v1
kind: ConfigMap
metadata:
name: kafka-broker-config
namespace: knative-eventing
data:
# Number of topic partitions
default.topic.partitions: "10"
# Replication factor of topic messages.
default.topic.replication.factor: "1"
# A comma separated list of bootstrap servers. (It can be in or out the k8s cluster)
bootstrap.servers: "my-cluster-kafka-bootstrap.kafka:9092"
```

The above `ConfigMap` is installed in the cluster, and you can edit
the configurations or create a new one with the same shape
depending on your needs.
Copy link
Contributor

@abrennan89 abrennan89 Aug 12, 2020

Choose a reason for hiding this comment

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

Let's be more prescriptive than this. Is it specifically yhe kafka-broker-config ConfigMap that needs to be referenced?
If we're going to talk about a previously unmentioned ConfigMap in the docs, it should probably be explained properly in its own section, as it is for the default broker: https://knative.dev/development/eventing/broker/config-br-defaults/

Copy link
Member Author

@pierDipi pierDipi Aug 13, 2020

Choose a reason for hiding this comment

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

Are you suggesting to create another page for this config map?

abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
abrennan89
abrennan89 reviewed Aug 12, 2020
View reviewed changes
@googlebot
Copy link

googlebot commented Aug 12, 2020

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot 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 12, 2020
@pierDipi pierDipi requested a review from abrennan89 August 13, 2020 12:34
@pierDipi
Copy link
Member Author

pierDipi commented Aug 13, 2020

@abrennan89 Thank you for the review, I left some questions and addressed most of your comments.

@pierDipi pierDipi changed the title [WIP] Add Apache Kafka Broker Add Apache Kafka Broker Aug 17, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 17, 2020
mpetason
mpetason reviewed Aug 18, 2020
View reviewed changes
@pierDipi
Copy link
Member Author

pierDipi commented Aug 18, 2020

Thanks @mpetason!

@abrennan89 abrennan89 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 18, 2020
@googlebot
Copy link

googlebot commented Aug 18, 2020

☹️ Sorry, but only Googlers may change the label cla: yes.

@googlebot googlebot removed the cla: yes Indicates the PR's author has signed the CLA. label Aug 18, 2020
@abrennan89
Copy link
Contributor

abrennan89 commented Aug 18, 2020

frowning_face Sorry, but only Googlers may change the label cla: yes.

@macruzbar @carieshmarie can one of you add this manually please, Thanks.

@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Aug 18, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrennan89, pierDipi

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:

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

@pierDipi
Add Apache Kafka Broker
45e1263 
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
@googlebot
Copy link

googlebot commented Aug 18, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 18, 2020
@abrennan89
Copy link
Contributor

abrennan89 commented Aug 18, 2020

/lgtm

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

Reviewers

4 more reviewers

@mpetason mpetason mpetason left review comments

@mattmoor mattmoor mattmoor left review comments

@slinkydeveloper slinkydeveloper slinkydeveloper left review comments

@abrennan89 abrennan89 abrennan89 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@abrennan89 abrennan89

Labels

cla: yes Indicates the PR's author has signed the CLA. 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pierDipi @googlebot @abrennan89 @knative-prow-robot @mpetason @mattmoor @slinkydeveloper