Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

Port KafkaChannel to new bindings API #1039

Merged

Conversation

slinkydeveloper
Copy link
Contributor

Fixes #976

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 18, 2020
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 18, 2020
@slinkydeveloper slinkydeveloper changed the title Kafka channel bindings Port KafkaChannel to new bindings API Mar 18, 2020
@matzew
Copy link
Member

matzew commented Mar 18, 2020

cool, I will run some tests here

@slinkydeveloper
Copy link
Contributor Author

/hold

I wish to fix the intermediate buffering that happens inside message receiver, which can be avoided for the kafka case

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2020
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2020
@slinkydeveloper
Copy link
Contributor Author

Depends on knative/eventing#2773

@knative-prow-robot knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 18, 2020
@slinkydeveloper
Copy link
Contributor Author

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 18, 2020
@lberk
Copy link
Member

lberk commented Mar 18, 2020

/assign

@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2020
@lberk
Copy link
Member

lberk commented Mar 18, 2020

Outside of those two nits this works for me
/lgtm
/hold

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Mar 18, 2020
@slinkydeveloper
Copy link
Contributor Author

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 19, 2020
@lberk
Copy link
Member

lberk commented Mar 19, 2020

@slinkydeveloper did you test these changes? With the last commit the dispatcher doesn't forward any messages anymore for me.

Seems like the correct approach (for now) is to keep the variables assigned below the NewMessageReciever and remove the topicFunc assignment within the struct declaration. Please make sure to remove the duplicate assignement (as it wasn't removed in 9fc5704

@slinkydeveloper
Copy link
Contributor Author

@slinkydeveloper did you test these changes? With the last commit the dispatcher doesn't forward any messages anymore for me.

Funny how the test pass...

Signed-off-by: Francesco Guardiani <[email protected]>
Signed-off-by: Francesco Guardiani <[email protected]>
Signed-off-by: Francesco Guardiani <[email protected]>
@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
kafka/channel/pkg/dispatcher/dispatcher.go 66.2% 61.1% -5.2

@slinkydeveloper
Copy link
Contributor Author

@lberk I've tested locally and it works :)

@lberk
Copy link
Member

lberk commented Mar 19, 2020

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2020
@n3wscott
Copy link
Contributor

/lgtm
/approve

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lberk, n3wscott, 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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 19, 2020
@knative-prow-robot knative-prow-robot merged commit f51f4da into knative:master Mar 19, 2020
@slinkydeveloper slinkydeveloper deleted the kafka_channel_bindings branch March 19, 2020 16:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port KafkaChannel to cloudevents/sdk-go bindings
7 participants