support partition consumer mode for kafka channel by yuzisun · Pull Request #879 · knative/eventing

yuzisun · 2019-03-09T00:07:38Z

Fixes #693

Proposed Changes

support partition consumer mode which allows dispatching events from different partitions in parallel instead of a single go channel.
added consumer_mode in kafka channel config, by default it is still doing multiplexing. When consumer_mode is set to partitions, kafka dispatcher should start creating partition consumer for the channels and events are dispatched on different go channels for different partitions.

Release Note

Added `consumer_mode` config option in config map `kafka-channel-controller-config`, default value is `multiplex` which retains existing behavior, if you set `consumer_mode` to be `partitions`,  it creates a go channel per partition for the consumer when dispatching the events to your service.

Harwayne

/ok-to-test

Harwayne · 2019-03-09T00:54:34Z

/ok-to-test

n3wscott

nice bit of work!

knative-metrics-robot · 2019-03-12T19:31:53Z

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
contrib/kafka/pkg/dispatcher/dispatcher.go	71.1%	69.9%	-1.2

matzew · 2019-03-13T15:30:11Z

/lgtm

nice idea.

Perhaps we should change this to be the default behavior, in the future

n3wscott · 2019-03-13T20:15:17Z

-			} else {
-				break
+	if cluster.ConsumerModePartitions == d.kafkaCluster.GetConsumerMode() {
+		go d.partitionConsumerLoop(consumer, channelRef, sub)


thanks a lot, this looks easier to test now.

n3wscott · 2019-03-13T20:15:32Z

/lgtm

grantr

@yuzisun Thanks, and thanks for writing unit tests! Did you verify this feature works in a real cluster? I'm not very familiar with the Kafka channel and there aren't any E2E tests yet, so I'd like to hear from at least one person who's seen this code work in real life 😀

Also, since this adds a feature to the Kafka channel config map, can you write a blurb about it in the Release Note block? Seems like it can be copied from your Proposed Changes list.

grantr · 2019-03-13T21:33:15Z

 	"go.uber.org/zap"
 	"k8s.io/apimachinery/pkg/runtime"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
+	"os"


Did you run gofmt on this file? I'd have expected it to put this import above next to flag.

hmm I did run gofmt -w main.go, nothing changes, then I purposely put next to flag and rerun gofmt it puts os back after _ k8s.io/client-go/plugin/pkg/client/auth/gcp, looks weird. I am using go version go1.11.5 darwin/amd64

weird! I don't know why it does that, but I guess it's correct by definition. So 👍

yuzisun · 2019-03-13T22:19:29Z

@yuzisun Thanks, and thanks for writing unit tests! Did you verify this feature works in a real cluster? I'm not very familiar with the Kafka channel and there aren't any E2E tests yet, so I'd like to hear from at least one person who's seen this code work in real life 😀

Also, since this adds a feature to the Kafka channel config map, can you write a blurb about it in the Release Note block? Seems like it can be copied from your Proposed Changes list.

Added the release note. I did verify this in the real cluster and I can also look into adding a E2E test for kafka.

grantr

Added the release note. I did verify this in the real cluster and I can also look into adding a E2E test for kafka.

Thanks! I took the liberty of moving the release note text into the ```releasenote block. E2E tests can be another PR.

knative-prow-robot · 2019-03-13T23:41:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, yuzisun

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

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

yuzisun · 2019-03-14T00:59:06Z

/retest

* support partition consumer for kafka channel * consolidate GetProvisionerConfig function * add consumer mode config test * address review comments

support partition consumer for kafka channel

0a01c5e

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 9, 2019

knative-prow-robot requested review from Harwayne and evankanderson March 9, 2019 00:07

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 9, 2019

Harwayne reviewed Mar 9, 2019

View reviewed changes

consolidate GetProvisionerConfig function

bf563f8

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 9, 2019

add consumer mode config test

add9b4a

yuzisun force-pushed the kafka_partition_consumer branch from 28b05a0 to add9b4a Compare March 9, 2019 15:09

n3wscott reviewed Mar 12, 2019

View reviewed changes

Comment thread contrib/kafka/main.go Outdated

Comment thread contrib/kafka/pkg/controller/util.go

Comment thread contrib/kafka/pkg/dispatcher/dispatcher.go

address review comments

31e122b

knative-prow-robot assigned matzew Mar 13, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2019

n3wscott reviewed Mar 13, 2019

View reviewed changes

knative-prow-robot assigned n3wscott Mar 13, 2019

grantr reviewed Mar 13, 2019

View reviewed changes

grantr approved these changes Mar 13, 2019

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2019

knative-prow-robot merged commit b7738a4 into knative:master Mar 14, 2019

matzew mentioned this pull request Mar 14, 2019

e2e test for Apache Kafka CCP #901

Closed

Conversation

yuzisun commented Mar 9, 2019 • edited by grantr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

Harwayne left a comment

Choose a reason for hiding this comment

Uh oh!

Harwayne commented Mar 9, 2019

Uh oh!

n3wscott left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented Mar 12, 2019

Uh oh!

matzew commented Mar 13, 2019

Uh oh!

n3wscott Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

n3wscott commented Mar 13, 2019

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

grantr Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

yuzisun Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

grantr Mar 13, 2019

Choose a reason for hiding this comment

Uh oh!

yuzisun commented Mar 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grantr left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Mar 13, 2019

Uh oh!

yuzisun commented Mar 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

yuzisun commented Mar 9, 2019 •

edited by grantr

Loading

yuzisun commented Mar 13, 2019 •

edited

Loading