asyncapi / bindings

AsyncAPI bindings specifications
Apache License 2.0
72 stars 75 forks source link

feat: apache Pulsar bindings #173

Closed VisualBean closed 1 year ago

VisualBean commented 2 years ago

Adds Server and channel bindings for Apache Pulsar

Related issue(s) Resolves #164

VisualBean commented 2 years ago

Is it allowed to collaborate on a PR here, or should we do it on the Fork? Id really like some early comments, before we fully go for it, but if it is against the contributing guidelines, please close :)

@Austek for Collab.

austek commented 2 years ago

sure we can collaborate, I'm on AsyncApi Slack

fmvilas commented 2 years ago

It's completely fine to collaborate here if you can give @austek permissions to edit this PR.

VisualBean commented 2 years ago

One big question is, Pulsar has tenants, as the "top" of the hierarchy.

In order to Produce to a given topic, a uri that looks like this is needed. {persistent|non-persistent}://tenant/namespace/topic

In the current iteration, ive attached Tenant to the Server binding, to try and reduce repitition, but what kind of flexibility are we looking for in the bindings?

In the usecases im looking at, AsyncApi documents are per tenant. @austek for additional input.

VisualBean commented 1 year ago

Left a comment. I have no idea about Pulsar so we have to fully trust you on this folks 😄

Also, would you be up for becoming the code owner of this binding? You would be listed at https://github.com/asyncapi/bindings/blob/master/CODEOWNERS and would become a TSC member.

I would love to.

VisualBean commented 1 year ago

Are we planning to add Pulsar bindings for message and operation AsyncAPI components?

I think the followings are also important to define in the pulsar bindings: AccessMode https://pulsar.apache.org/docs/2.10.x/concepts-messaging#access-mode

Subscription Type https://pulsar.apache.org/docs/2.10.x/concepts-messaging#subscription-types

Subscription Mode https://pulsar.apache.org/docs/2.10.x/concepts-messaging#subscription-modes

Although we can add more Pulsar features in the next iterations of binding spec, it would be great if we can capture the major features in the first version. FYI, I notified the Pulsar community about this work in this email thread, https://lists.apache.org/thread/q8hlg27c1yh94w73j53dy2y54yyjdt62

You are very welcome to put suggestions, but maybe some of the subscription specific things, fit the operations binding. I couldn't immediately think of anything that fits a message binding, and haven't found something particularly cluster setup wise that fits the operation binding, as most of those things are dynamically configurable from the clients, and not part of the namespace/topic setup, but if i completely missed something, let's get them going as well.

Access mode to my knowledge is the client, not the topic setup on the Pulsar end. correct me if I'm wrong, but it isn't set until there is a producer, and thus cannot be denoted the binding, as another producer claiming the access mode could change it any time. Unlike, compaction, retention, deduplication etc. Which are defined on the topic specifically.

And its kind of the same story for the subscription specific things you mention. The asyncapi document cannot really define the actual subscriptions to topics as those in most cases are dynamically created. Or perhaps i am misunderstanding you intent.

heesung-sn commented 1 year ago

I think we could first try to agree on the definitions of server, channel, operation, and message bindings in the Pulsar context.

From my understanding, we are going with the below proposal in this PR?

server: tenant level broker-admin configurations. channel: namespace/topic level broker-admin configurations. operation: pub/sub client level configurations. message: message level configs(?) -- not sure about this message bindings for pulsar(perhaps TBD)

VisualBean commented 1 year ago

server: tenant level broker-admin configurations. channel: namespace/topic level broker-admin configurations. operation: pub/sub client level configurations. message: message level configs(?) -- not sure about this message bindings for pulsar(perhaps TBD)

Yes, that is my take as well, although, tenant could feasibly be channel level, im a bit up in the air about that one.

For the first version, I wanted to leave out all of the client dynamic configuration things, so essentially trying to stick to what can be set through the REST API, if that makes sense, as that configuration is bound by the cluster, and not by the clients.

Having thought a bit about it, I think dead-letter-queue could make sense, as well as some other producer/consumer specifics, but perhaps those can be in a future version, as they are dynamically configurable by the clients producing to the topics in question, and as such, are much harder to determine "accurate" configuration of.

heesung-sn commented 1 year ago

server: tenant level broker-admin configurations. channel: namespace/topic level broker-admin configurations. operation: pub/sub client level configurations. message: message level configs(?) -- not sure about this message bindings for pulsar(perhaps TBD)

Yes, that is my take as well, although, tenant could feasibly be channel level, im a bit up in the air about that one.

For the first version, I wanted to leave out all of the client dynamic configuration things, so essentially trying to stick to what can be set through the REST API, if that makes sense, as that configuration is bound by the cluster, and not by the clients.

Having thought a bit about it, I think dead-letter-queue could make sense, as well as some other producer/consumer specifics, but perhaps those can be in a future version, as they are dynamically configurable by the clients producing to the topics in question, and as such, are much harder to determine "accurate" configuration of.

Ok. this direction makes sense to me too. We could stick to the broker-admin configs first in this PR.

Also, server: cluster/tenant level broker-admin configs make sense to me too in this first version.

lin-zhao commented 1 year ago

Do we also need binding for messages? If my understanding is correct, message binding is what's needed for defining schemas right?

austek commented 1 year ago

How about schema, shouldn't it be part of the API spec?

VisualBean commented 1 year ago

Do we also need binding for messages? If my understanding is correct, message binding is what's needed for defining schemas right?

No, you can specify the schema as part of the message object in the asyncapi document.

VisualBean commented 1 year ago

@fmvilas I think we are somewhat stable at this point. How do we procede?

VisualBean commented 1 year ago

yo, left some editorial comments

➕ I have some other comments

* in `bindings` repo we always ask binding contributors if they want to become given binding maintainers. Of course, remember that you are not left alone, and main spec maintainers can always jump in and help if there is something that needs to be cleared out and consulted. So are you willing to maintain it? especially that I see there will be some followup PRs to add bindings to other objects.

* I assume you'd like to use January release window to get `pulsar` on the list of bindings in the main spec? We need to know as we need to start looking for volunteer to coordinate 2.6 release

anyway man, great PR 👏🏼 also, looking at the number of comments, I'm guessing the Pulsar community was really looking forward to this one 💪🏼

fmvilas commented 1 year ago

Let's wait for other reviews \cc @derberg @dalelane @char0n

derberg commented 1 year ago

I am very willing to maintain it.

Awesome! thanks so much. Then please update https://github.com/asyncapi/bindings/blob/master/CODEOWNERS in your PR 🙏🏼

January release would be perfect.

How much do you plan to engage with AsyncAPI community. Are you looking for something more than just pulsar binding?

I'm asking because the specification releases are always coordinated by a release coordinator. This is a volunteer role that anyone can pick up. I'm not gonna lie; it ain't easy and requires some work. You can read more here. But the big advantage is that you learn a lot about how AsyncAPI is organized and tooling ecosystem and get in touch with different maintainers around. And yeah, we are always here to support you of course.

Give it a thought.

VisualBean commented 1 year ago

I am very willing to maintain it.

Awesome! thanks so much. Then please update https://github.com/asyncapi/bindings/blob/master/CODEOWNERS in your PR 🙏🏼

January release would be perfect.

How much do you plan to engage with AsyncAPI community. Are you looking for something more than just pulsar binding?

I'm asking because the specification releases are always coordinated by a release coordinator. This is a volunteer role that anyone can pick up. I'm not gonna lie; it ain't easy and requires some work. You can read more here. But the big advantage is that you learn a lot about how AsyncAPI is organized and tooling ecosystem and get in touch with different maintainers around. And yeah, we are always here to support you of course.

Give it a thought.

I am building a dotnet library that reads/writes asyncapi documents based on the objectmodel, which will go opensource very soon (hopefully) as well, so I am hoping to engage more, but am not ready to take on more work unfortunately.

I have added myself as a codeowner of the pulsar folder.

VisualBean commented 1 year ago

Can someone rerun the sentiment analysis, looks like its failing due to a failure in the imported action "Error: Unable to resolve action someimportantcompany/github-actions-slack-message@v1, unable to find version v1"

fmvilas commented 1 year ago

@dalelane can you please re-approve if you agree with the changes?

fmvilas commented 1 year ago

/rtm

fmvilas commented 1 year ago

Congrats and thanks @VisualBean! I guess you now have to create a PR in the spec to add the pulsar binding and it will make it to the January release :)