Skip to content

Apache Pulsar Transport Binding#237

Closed
sijie wants to merge 2 commits into
cloudevents:masterfrom
sijie:apache_pulsar_transport_binding
Closed

Apache Pulsar Transport Binding#237
sijie wants to merge 2 commits into
cloudevents:masterfrom
sijie:apache_pulsar_transport_binding

Conversation

@sijie
Copy link
Copy Markdown

@sijie sijie commented Jun 18, 2018

Motivation

Apache Pulsar is a distributed pub/sub messaging system.
It would be great to define a transport binding for CloudEvents on
how to map between pulsar messages and CloudEvents.

Changes

This PR proposes an Apache Pulsar
transport binding for CloudEvents, similar to other existing transport bindings.

Signed-off-by: Sijie Guo sijie@apache.org

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jun 20, 2018

CI failure, while real, is ok since it'll be automatically fixed when we merge.

@sijie
Copy link
Copy Markdown
Author

sijie commented Jun 20, 2018

thank you @duglin . do I need to fix it now?

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jun 20, 2018 via email

Comment thread apache-pulsar-transport-binding.md Outdated

properties: {
CE-CloudEventsVersion: "0.1"
CE-EventType: "org.apache.pulsar.someevent"
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.

spacing on this line seems off

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.

do you need quotes around the names?

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.

yes. we need quotes around the names, since the names are strings.

added quotes and fix the spacing.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jun 20, 2018

Minor thing, but can you wrap the text at 80 columns? Most of this does do that but there are places where it doesn't - just so we're consistent.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jun 20, 2018

Can you modify the Makefile so this md file is run thru the verify-specs.sh checker?

@sijie
Copy link
Copy Markdown
Author

sijie commented Jun 20, 2018

but can you wrap the text at 80 columns?

wrapped the text at 80 columns

Can you modify the Makefile so this md file is run thru the verify-specs.sh checker?

verify-specs.sh seems to glob the files suffixed with .md already.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jun 20, 2018

verify-specs.sh is called with a fixed list of md file in the Makefile since not all md files need RFC2119 keywords checked.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Jun 20, 2018

Make sure you sign all of our commits -the DCO checker isn't happy. And thanks for the quick fixes

*Motivation*

Apache Pulsar is a distributed pub/sub messaging system.
It would be great to define a transport binding for CloudEvents on
how to map between pulsar messages and CloudEvents.

*Changes*

This PR proposes an [Apache Pulsar](https://pulsar.incubator.apache.org/)
transport binding for CloudEvents, similar to other existing transport bindings.

Signed-off-by: Sijie Guo <sijie@apache.org>

Wrap the text at 80 columns

add quotes around the names
@sijie sijie force-pushed the apache_pulsar_transport_binding branch from 24477af to 9b0304e Compare June 20, 2018 19:59
@sijie
Copy link
Copy Markdown
Author

sijie commented Jun 20, 2018

gotcha. thank you @duglin

Signed-off-by: Sijie Guo <sijie@apache.org>
@sijie sijie force-pushed the apache_pulsar_transport_binding branch from 507df7e to d60dc69 Compare June 20, 2018 20:10
@clemensv
Copy link
Copy Markdown
Contributor

Per discussion last week, created this PR #254

Pulsar is using a project-proprietary protocol.

The payload mapping really ought to be a Protobuf event format, which would be good to work on with Google.

I think it'd be great for Pulsar to define and document how to use CloudEvents with Pulsar inside its own project, but it appears doubtful that there are implementers unrelated to the Pulsar project who would pick up the Pulsar binding spec and implement a clean Pulsar protocol binding for interoperability with others.

@sijie
Copy link
Copy Markdown
Author

sijie commented Jun 27, 2018

@clemensv thank you for your comment.

some thoughts regarding Protobuf event format:

  1. pulsar protocol is not purely protobuf format. pulsar only uses protobuf for encoding message headers and leave the message data out of protobuf, because protobuf has a lot of memory copies on handing bytes and string, which would kill the performance. most of the data related projects I know using protobuf are trying to avoid putting bytes as part of protobuf messages, which they end up with project-proprietary protocol.

  2. I think it is make sense to define a CloudEvent format using protobuf, which we can send a more binary-compacted structure over the wire and be able to exchange between systems. However for a transport that has its own transport protocol, its focus would be 1) how to map its existing transport messages/format to cloud events format natively 2) how a transport can use its protocol to ship cloud events data. these are kind of different from defining a protobuf cloud event format.

Hope my thoughts about protobuf event format make sense here. If I missed something regarding spec, please point them out. Happy to learn more about the spec.

I think it'd be great for Pulsar to define and document how to use CloudEvents with Pulsar inside its own project

I can't speak for the whole pulsar community or the users of pulsar. but from the use cases I can see, cloud events can be used in following use cases in pulsar:

  1. Pulsar I/O. Pulsar I/O is a serverless-inspired connector framework that connect pulsar with other systems (e.g. databases, storage and such). If the source and sink that pulsar connects to provides or accepts cloud events formated events, it simplifies pulsar connector developers, which it doesn't have to worry about format transformations between different systems that pulsar connects to.

  2. Pulsar Functions. Pulsar Functions is a lightweight computing framework that provides in-stream processing, it process events received from source connectors and produce the results to sink connector. users write a function to process the events. so it would have same benefits as pulsar I/O.

These are the two usages within Pulsar itself. However I am not sure if we should put them in the transport binding spec. I am open to any suggestions.

@clemensv
Copy link
Copy Markdown
Contributor

@sijie My point is that a transport binding spec for CloudEvents is only useful when it is that there will be multiple and potentially competing efforts that will achieve interoperability by implementing a transport binding for CloudEvents based on that spec. I don't see that being the case here since the protocol is project-proprietary.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Sep 13, 2018

@sijie there's some concern that the Pulsar spec doesn't meet the minimum bar we've defined for a new protocol/binding - see: https://github.com/cloudevents/spec/blob/master/primer.md#qualifying-protocols-and-encodings I'd be interested in getting your take on it since this will probably come up on this week's call.

@sijie
Copy link
Copy Markdown
Author

sijie commented Sep 13, 2018

@duglin sorry just saw your messages. the concerns make sense to me. feel free to close it.

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Sep 13, 2018

ok, we'll wait until next week's call to see if anyone has any objections to closing it

@duglin
Copy link
Copy Markdown
Collaborator

duglin commented Sep 20, 2018

Agreed to close it in the 9/20 call.
If someone would like to champion this in the future we can reopen.

@duglin duglin closed this Sep 20, 2018
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.

3 participants