Kafka transport binding by longit644 · Pull Request #300 · cloudevents/spec

longit644 · 2018-09-01T03:20:13Z

I'd like to propose a Kafka transport binding for CloudEvents, similar to the HTTP binding and proposed NATS, MQTT, AMQP bindings.

Signed-off-by: Long Bui bnlong@fossil.com

duglin · 2018-09-01T19:54:39Z

+
+## Abstract
+
+The [Kafka][Kafka] Transport Binding for CloudEvents defines how events are mapped to [Kafka messages][kafka-Message-Format].


please wrap at 80 cols

duglin · 2018-09-01T19:56:16Z

CI error is ok and will automatically be fixed once merged.

Signed-off-by: Long Bui <bnlong@fossil.com>

duglin · 2018-09-07T01:09:31Z

+placed into the Kafka message value section
+using an [event format](#14-event-formats).
+
+In the *binary* content mode, the value of the event `data` attribute is placed


I wonder if we need an RFC2119 keyword in here, perhaps s/is placed/MUST be placed/ ??

duglin · 2018-09-07T01:09:56Z

+In the *binary* content mode, the value of the event `data` attribute is placed
+into the Kafka message's value section as-is, with
+the `kafka_contentType` header value declaring its media type; all other event
+attributes are mapped to the Kafka message's


maybe: s/are mapped/MUST be mapped/

duglin · 2018-09-07T01:11:17Z

+With Kafka 0.11.0.0 and above, the content mode is chosen by the sender of the 
+event. Protocol usage patterns that might allow solicitation of events using a
+particular content mode might be defined by an application, but are not defined
+ here.


extra space before "here"

duglin · 2018-09-07T01:13:11Z

+#### 3.1.3. Metadata Headers
+
+All [CloudEvents][CE] attributes with exception of `data` MUST be individually
+mapped to and from the Header fields in the Kafka message.


Is there any rule needed w.r.t. the names used? Case of the names?

duglin · 2018-09-07T01:14:00Z

+
+##### 3.1.3.1 User Property Names
+
+Cloud Event attributes are prefixed with "cloudEvents_" for use in the


extra space between "Cloud" and "Event"

duglin · 2018-09-07T01:14:28Z

+All [CloudEvents][CE] attributes with exception of `data` MUST be individually
+mapped to and from the Header fields in the Kafka message.
+
+##### 3.1.3.1 User Property Names


What is the difference between "User Properties" and "Metadata Headers" ?

duglin · 2018-09-07T01:17:11Z

+the Kafka `kafka_contentType` header; all other CloudEvents attributes
+are mapped to Kafka Header fields with prefix `cloudEvents_`.
+
+Mind that `cloudEvents_` here does refer to the event `data`


I'm not sure I understand this line, what do you mean by "cloudEvents_ here" ?

cloudEvents_ is the prefix of CloudEvents context attributes, I want to mark these attributes as CloudEvents context attributes, except for contentType because it's a common attribute.

duglin · 2018-09-07T01:18:26Z

+The chosen [event format](#14-event-formats) defines how all attributes,
+including the `data` attribute, are represented.
+
+The event metadata and data is then rendered in accordance with the event


s/is/are/ I think

should we be using from RFC2119 keywords in here some place?

duglin · 2018-09-07T01:19:37Z

+
+#### 3.2.1. Kafka Content-Type
+
+The [Kafka `kafka_contentType`] property field is set to the media type


maybe s/is set/MUST be set/

duglin · 2018-09-07T01:21:07Z

+format][Kafka-Message-Format] specification.
+
+
+#### 3.1.4 Examples


nit but since there's just one example you can remove the 's' at the end

duglin · 2018-09-07T01:21:22Z

+Implementations MAY include the same Kafka headers as defined for the
+[binary mode](#313-metadata-headers).
+
+#### 3.2.4 Examples


just one example

duglin · 2018-09-07T01:23:30Z

overall this looks good to me, but I'm not a Kafka expert, and I think my questions/edits are relatively minor. It does seem like there might be some spots where more RFC2119 keywords should be used, so perhaps a Kafka expert read this with an eye for that.

duglin · 2018-09-13T20:30:46Z

@bnlong4417 can you address the comments in here - then we may be able to resolve this on next week's call

clemensv · 2018-09-18T17:52:47Z

I'm in generally in favor of having such a binding, and haven't yet been able to make the time to review this proposal side-by-side with the Kafka protocol spec, but promise to do so this week.

I would like to use this PR to close some @duglin assigned homework, namely the AI related to issue #218 .

We should have an explicit mechanism in here that states how to derive the partitionKey that Kafka needs and that this proposal here doesn't yet seem to mention except in the example. I don't have a proposal yet for how to do that.

jroper · 2018-09-20T00:47:47Z

+The receiver of the event can distinguish between the two content modes by
+inspecting the `kafka_contentType` property of the Kafka message. If the value
+of the `kafka_contentType` property is prefixed with the CloudEvents media type
+`application/cloudevents`, indicating the use of a known [event


If I'm not mistaken, this has been copied from the HTTP binding, which means we now have two different specs specifying this MIME type. That's not ideal, it should only be specified in one place. So I think this should be added to the main spec to say what its MIME type is, and then both this and the HTTP binding can reference that, plus the JSON spec should be the authoritative spec that says that it is the format for application/cloudevents+json, not this and the HTTP binding.

Yes, I borrowed it from HTTP specs. I think it should be moved to main specs too.

jroper · 2018-09-20T00:54:49Z

+
+In the *binary* content mode, the value of the event `data` attribute is placed
+into the Kafka message's value section as-is, with
+the `kafka_contentType` header value declaring its media type; all other event


Where does kafka_contentType come from? As far as I can work out, this is unique to this spec. While there isn't a standard header in Kafka for content type, it does appear that there are libraries out there that do put a content type header, and it appears to me that there is some consensus around contentType. Even without that, I don't think kafka_contentType makes sense, since of course it's Kafka. If this header is specific to CloudEvents, and you want to namespace it, then the prefix should be couldevents, since a CloudEvents spec like this has no business defining a header in the kafka namespace.

Currently, there isn't a standard header in Kafka for the content type. So I used the Spring's format for kafka_contentType at here: https://github.com/spring-projects/spring-kafka/blob/996f8649d562cb7a2b15af42ab0bea64b510d961/spring-kafka/src/main/java/org/springframework/kafka/support/KafkaHeaders.java#L32

duglin · 2018-09-20T12:47:02Z

@bnlong4417 there are some comments/questions - can you try to address those? We can't consider adopting this until those are addressed.

duglin · 2018-10-15T14:55:58Z

@bnlong4417 are you still interested in us adopting this PR? There are some updates needed if so.

duglin · 2018-10-16T17:23:46Z

@bnlong4417 we haven't heard from you since you opened this PR. Please speak-up before this Thursday's call if you're interested in continuing with this - otherwise I'm going to ask if there's someone else who would like to own it - and if not, then I'm going to propose that we close it. We can always reopen later if needed.

duglin · 2018-10-18T20:25:13Z

@bnlong4417 on today's call Neil Avery & David Baldwin agreed to carry this forward - hope that's ok with you.

longit644 · 2018-10-19T05:39:17Z

@duglin My sincere apologies for the slow reply. I am very busy at the moment because our company is preparing for the important holiday sales this year and I forgot to catch this up. I'm totally ok with Avery & Baldwin to carry this forward, thank you for continuing to integrate Kafka into CloudEvents.

duglin · 2018-10-19T10:51:40Z

@bnlong4417 no worries - thanks for getting back to us.... I was a little worried that you vanished :-)

bluemonk3y · 2018-10-23T13:30:02Z

@duglin - I will have the follow-on PR available next week - I'm currently OOO - regards Neil

duglin · 2018-11-01T14:28:21Z

Given #337 is ok to close this one?

duglin · 2018-11-01T18:32:09Z

I'm going to assume we can close this due to #337 is now out there. I can reopen if someone things I jumped the gun.

* Follow-on PR from #300 Kafka transport binding for CloudEvents, similar to the HTTP binding and proposed NATS, MQTT, AMQP bindings. Signed-off-by: Neil Avery <neil@confluent.io> * addressing remaining cloudEvent attribute names Kafka transport binding for CloudEvents, similar to the HTTP binding and proposed NATS, MQTT, AMQP bindings. Signed-off-by: Neil Avery <neil@confluent.io>

duglin reviewed Sep 1, 2018

View reviewed changes

Add event transport binding for Kafka.

1be3825

Signed-off-by: Long Bui <bnlong@fossil.com>

duglin reviewed Sep 7, 2018

View reviewed changes

jroper reviewed Sep 20, 2018

View reviewed changes

bluemonk3y mentioned this pull request Nov 1, 2018

Kafka transport binding v2 #337

Merged

duglin closed this Nov 1, 2018


		## Abstract

		The [Kafka][Kafka] Transport Binding for CloudEvents defines how events are mapped to [Kafka messages][kafka-Message-Format].


		##### 3.1.3.1 User Property Names

		Cloud Event attributes are prefixed with "cloudEvents_" for use in the


		#### 3.2.1. Kafka Content-Type

		The [Kafka `kafka_contentType`] property field is set to the media type

		format][Kafka-Message-Format] specification.


		#### 3.1.4 Examples

Conversation

longit644 commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Sep 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

longit644 Oct 19, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Sep 7, 2018

Uh oh!

duglin commented Sep 13, 2018

Uh oh!

clemensv commented Sep 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

longit644 Oct 19, 2018 • 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!

duglin commented Sep 20, 2018

Uh oh!

duglin commented Oct 15, 2018

Uh oh!

duglin commented Oct 16, 2018

Uh oh!

duglin commented Oct 18, 2018

Uh oh!

longit644 commented Oct 19, 2018

Uh oh!

duglin commented Oct 19, 2018

Uh oh!

bluemonk3y commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duglin commented Nov 1, 2018

Uh oh!

duglin commented Nov 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

longit644 commented Sep 1, 2018 •

edited

Loading

longit644 Oct 19, 2018 •

edited

Loading

longit644 Oct 19, 2018 •

edited

Loading

bluemonk3y commented Oct 23, 2018 •

edited

Loading