Skip to content

RocketMQ transport binding#352

Closed
zongtanghu wants to merge 8 commits into
cloudevents:masterfrom
zongtanghu:master
Closed

RocketMQ transport binding#352
zongtanghu wants to merge 8 commits into
cloudevents:masterfrom
zongtanghu:master

Conversation

@zongtanghu
Copy link
Copy Markdown

I commited the rocketmq-transport-binding.md for CloudEvents.The issues is #351.

@zongtanghu zongtanghu changed the title I commited the rocketmq-transport-binding.md for CloudEvents. RocketMQ transport binding Nov 23, 2018
…to the proposed NATS, MQTT, AMQP bindings.

Signed-off-by: huzongtang <huzongtang@cmss.chinamobile.com>
…to the proposed NATS, MQTT, AMQP bindings.

Signed-off-by: huzongtang <huzongtang@cmss.chinamobile.com>
…to the proposed NATS, MQTT, AMQP bindings.

Signed-off-by: huzongtang <huzongtang@cmss.chinamobile.com>
…to the proposed NATS, MQTT, AMQP bindings.

Signed-off-by: huzongtang <huzongtang@cmss.chinamobile.com>
@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Nov 24, 2018

@zongtanghu can you please add this md file to the "verify" step in the Makefile so the verify-specs.sh script is run on it?

…to the proposed NATS, MQTT, AMQP bindings.

Signed-off-by: huzongtang <huzongtang@cmss.chinamobile.com>
…to the proposed NATS, MQTT, AMQP bindings.

Signed-off-by: huzongtang <huzongtang@cmss.chinamobile.com>
@zongtanghu
Copy link
Copy Markdown
Author

Hi @duglin ,I have already add this md file to the "verify" step in the Makefile.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Nov 24, 2018

Thanks

@zongtanghu
Copy link
Copy Markdown
Author

You're welcome.Please help to review RocketMQ transport binding document.Thank you very much.

Comment thread Makefile Outdated
@tools/verify-specs.sh -v spec.md documented-extensions.md json-format.md \
http-transport-binding.md http-webhook.md mqtt-transport-binding.md \
nats-transport-binding.md protobuf-format.md
nats-transport-binding.md protobuf-format.md rocketmq-transport-binding.md
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can you wrap this at 80 cols?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay I will fix it.

Comment thread rocketmq-transport-binding.md Outdated

## Abstract

The [RocketMQ][RocketMQ] Transport Binding for CloudEvents defines how events are mapped to RocketMQ messages.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please wrap this doc at 80 cols.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay,I will fix it in a new commit.


### 1.3. Content Modes

The specification defines two content modes for transferring events:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This feels a bit odd to say we're going to define 2 modes but then say only one of them is supported. I'd prefer to not confuse people by only talking about the one we do support.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay,I will rewrite this paragraph.

The receiver of the event can distinguish between the two content modes by inspecting
the `CE_contentType` property of the RocketMQ message. If the value is prefixed with the
CloudEvents media type `application/cloudevents`, indicating the use of a known event format,
the receiver uses structured mode, otherwise it defaults to binary mode.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Again, let's drop the notion of binary since we don't support it. Then we can just mandate, with a MUST, what CE_contentType's value needs to be.

Comment thread rocketmq-transport-binding.md Outdated

Examples:

* `eventTime` maps to `CE_eventTime`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These need to use the new names we just adopted (time, id, specversion)

that it cannot handle, for instance `application/cloudevents+avro`, it MAY still treat the event
as binary and forward it to another party as-is .

### 3.1. Binary Content Mode
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'd drop this entire section since we're not supporting it.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Binary mode is used in rocketmq message header.So,I write the "Binary Content Mode" Here.


## 3. RocketMQ Message Mapping
The receiver of the event can distinguish between the two content modes by inspecting
the `CE_contentType` property of the RocketMQ message. If the value is prefixed with the
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is CE_contentType the cloudevent property or one defined by RocketMQ? If it's the CE one then I don't think it is used to determine binary vs structured.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Again,binary mode is used in rocketmq message header.Structured content mode is used for RocketMQ message body.So,we put the cloudevent property into the message body to determine the message contentType.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From looking at: https://github.com/cloudevents/spec/pull/352/files#diff-b1ac390f402e79182f0aa7a46737eb90R198 something doesn't feel right. On line 198 you've pulled out one of the CE properties (contenttype) as special and placed it in the "userproperties" section but I don't think that's correct. "contenttype" is meant to describe the format of "data" not of the "payload" which is how you've used it. So, let me ask again... is "CE_contenttype" the cloudevents property or one defined by RocketMQ? I believe it should be defined by RocketMQ and NOT the CloudEvents one since at that point in the processing we don't even know we have a cloudEvent at all, so prefixing a property with "CE_" feels premature.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@duglin We need a 'CE_contenttype' field to ensure the receiver of the event can distinguish between the two content modes by inspecting the 'CE_contenttype' property of the RocketMQ message. and this is a property can be set according to the data format of CloudEvents.


[CE]: ./spec.md
[JSON-format]: ./json-format.md
[RocketMQ]: http://rocketmq.apache.org/
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you make this a direct pointer to the messaging format and not the project? I can't seem to find the format itself in the docs.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Author

@zongtanghu zongtanghu Nov 26, 2018

Choose a reason for hiding this comment

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

Later,we will put the detailed messaging format on the direct pointer Link which is on our RocketMQ website.

Comment thread rocketmq-transport-binding.md Outdated
Example for the JSON format:

```
content-type: application/cloudevents+json; charset=UTF-8
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the key doesn't match what's specified on line 168

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks,I will fix it.

Comment thread rocketmq-transport-binding.md Outdated

------------------ user properties -------------------

CE_contentType: application/cloudevents+json; charset=UTF-8
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Isn't there some RocketMQ defined header that should hold this value?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Year,I think Clound Event properties value hold this values more suitable rather than RocketMQ defined header.

…to the proposed NATS, MQTT, AMQP bindings.

Signed-off-by: huzongtang <huzongtang@cmss.chinamobile.com>
Comment thread rocketmq-transport-binding.md Outdated
specification MUST support the [JSON event format][JSON-format].

### 1.5. Security
This specification does not introduce any new security features for RocketMQ, or mandate specific existing features to be used.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please wrap at 80. There are other lines down below too.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay,I will fix these issues.

@clemensv
Copy link
Copy Markdown
Contributor

RocketMQ is a project-proprietary transport, is it not?

Signed-off-by: huzongtang <huzongtang@cmss.chinamobile.com>
@zongtanghu
Copy link
Copy Markdown
Author

RocketMQ is a project-proprietary transport, is it not?

@clemensv Apache RocketMQ™ is an open source distributed messaging and streaming data platform.

@duhengforever
Copy link
Copy Markdown

@clemensv RocketMQ is a top level project in Apache Foundation, and it has been adopted by hundreds of companies, it‘s official website:https://rocketmq.apache.org/

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Nov 29, 2018

@zongtanghu we discussed this on today's call and there was some concern that RocketMQ might not meet the minimum bar we've defined for new protocols. In particular, if you look in the primer you'll see this section:


The CloudEvents effort shall not become a vehicle to even implicitly endorse or promote project- or product-proprietary protocols, because that would be counterproductive towards CloudEvents' original goals.

For a protocol or encoding to qualify for a core CloudEvents event format or protocol binding, it must belong to either one of the following categories:

  • The protocol has a formal status as a standard with a widely-recognized multi-vendor protocol standardization body (e.g. W3C, IETF, OASIS, ISO)
  • The protocol has a "de-facto standard" status for its ecosystem category, which means it is used so widely that it is considered a standard for a given application. Practically, we would like to see at least one open source implementation under the umbrella of a vendor-neutral open-source organization (e.g. Apache, Eclipse, CNCF, .NET Foundation) and at least a dozen independent vendors using it in their products/services.

Can you comment on whether you think RocketMQ satisfies either one of those bullets? Since I don't think RocketMQ has been taken to an SDO, it's really the 2nd bullet that we'd like to hear your thoughts on. There was some concern that if there is just one implementation of this protocol then the interoperability aspect of our goals might not be met.

@duhengforever
Copy link
Copy Markdown

duhengforever commented Nov 30, 2018

@duglin thanks for your comment and we are very grateful for your work.

But firstly, RocketMQ not only is a TLP of Apache Foundation, but also IMHO, it has been to be a de-facto standard in the field of e-commerce and finance, especially in China,it has been used by hundreds of companies in their production environment,its popularity has exceeded some standards in the messaging field that you are talking about.

Secondly, RocketMQ is the first implementation of OpenMessaing which is a vendor-neutral specification in the messaging field and it has been supported by a dozen independent vendors or cloud vendors.

So we think we meet the second bar list above, and we do the binding for the purpose of promoting the serverless ecology and the messaging ecology, and hope to describing event data in a common way in the messaging field, and provide more support for the severless ecology, so we think that these bars don’t make much sense. and we also don't need to make a binding with CE to enhance our brand because it doesn't make sense, and in my opinion, bar should be used to help with the ecological construction, rather than simply raising the threshold, after all, these have great subjective factors.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Dec 6, 2018

@zongtanghu on today's call we chatting about this and there were mixed feeling on it. However, @JemDay did have an interesting idea of creating a home for protocols that are in a similar situation. A home for them to be hosted and developed, but not yet made into an official CloudEvents approved specification. Not unlike how we have our "documented extensions". It could also be a good testing ground to see how much community interest there is in it. Anyway, @rachelmyers agreed to take a first pass at creating a proposal to see how the group feels about that direction. It is still possible that this PR get adopted "as is", or we may choose to direct it toward this other category of transport docs - we just don't know yet, but we wanted to explore this path before we made a final decision.

@JemDay ping @rachelmyers if you're interested in collaborating on the proposal.

@zongtanghu
Copy link
Copy Markdown
Author

@duglin Thanks for your help.

@duhenglucky
Copy link
Copy Markdown
Contributor

@JemDay @rachelmyers Welcome to help us complete this pr, and also explore a way for other projects to make the ecosystem of cloudevents more perfect.

@duhenglucky
Copy link
Copy Markdown
Contributor

@duglin Could we push this PR move on together :)

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jan 15, 2019

@duhenglucky we're waiting for @rachelmyers's PR before we move forward on this one. @rachelmyers any update or ETA?

@duhenglucky
Copy link
Copy Markdown
Contributor

@rachelmyers Hi, Could we work together to move forward on this PR? I think it's a good start for CloudEvents ecosystem.

@clemensv
Copy link
Copy Markdown
Contributor

See #351 (comment)

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Mar 16, 2019

With #380 now resolved, I believe that the WG will probably request that this PR be modified to add a link to this spec from the new proprietary-specs doc and that it be hosted elsewhere. I will add this to the next call's agenda for confirmation.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Mar 16, 2019

If you have an opinion on this topic (see previous comment) please either join the next weekly call or add a comment in this PR prior to the call to ensure your thoughts are taken into account.

@duhenglucky
Copy link
Copy Markdown
Contributor

@duglin @clemensv Thanks for your work, we believe that this decision is fair and sustainable. and we also support the decision, so @zongtanghu could we work together to modify this PR to provide a link?

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Mar 21, 2019

On today's call we agreed that this specification should follow the new process we've defined for proprietary specifications - plus the originators of this PR agreed with that direction. So, with that, I'm going to close this PR. @zongtanghu please open up another PR to modify the proprietary-specs.md document so it points to your hosted specification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants