Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: apache Pulsar bindings #173

Merged
merged 26 commits into from
Dec 9, 2022
Merged

Conversation

VisualBean
Copy link
Collaborator

@VisualBean VisualBean commented Nov 10, 2022

Adds Server and channel bindings for Apache Pulsar

Related issue(s)
Resolves #164

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

@VisualBean VisualBean marked this pull request as draft November 10, 2022 15:49
@VisualBean
Copy link
Collaborator Author

VisualBean commented Nov 10, 2022

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
Copy link

austek commented Nov 10, 2022

sure we can collaborate, I'm on AsyncApi Slack

@fmvilas
Copy link
Member

fmvilas commented Nov 14, 2022

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

@VisualBean VisualBean changed the title WIP: feat: Apache Pulsar bindings feat: Apache Pulsar bindings Nov 16, 2022
@VisualBean VisualBean marked this pull request as ready for review November 16, 2022 08:27
@VisualBean VisualBean changed the title feat: Apache Pulsar bindings feat: apache Pulsar bindings Nov 16, 2022
pulsar/README.md Outdated Show resolved Hide resolved
pulsar/README.md Outdated Show resolved Hide resolved
Moved things away from the Server binding, as they werent attached to the Server.
@VisualBean
Copy link
Collaborator Author

VisualBean commented Nov 17, 2022

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.

pulsar/json_schemas/server.json Outdated Show resolved Hide resolved
pulsar/json_schemas/channel.json Show resolved Hide resolved
pulsar/json_schemas/channel.json Show resolved Hide resolved
pulsar/json_schemas/channel.json Show resolved Hide resolved
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

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.

pulsar/README.md Outdated Show resolved Hide resolved
@VisualBean
Copy link
Collaborator Author

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.

Copy link

@heesung-sn heesung-sn left a comment

Choose a reason for hiding this comment

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

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

pulsar/json_schemas/server.json Show resolved Hide resolved
pulsar/json_schemas/channel.json Show resolved Hide resolved
@VisualBean
Copy link
Collaborator Author

VisualBean commented Nov 21, 2022

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.

@VisualBean
Copy link
Collaborator Author

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 💪🏼

  • I am very willing to maintain it.
  • There might not be a need for operation and message bindings, we will see. Pulsar takes care of most of that behind the scenes, so I have yet to uncover an actual need for these, that arent already described by the asyncapi specification.
  • January release would be perfect.

Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
pulsar/README.md Outdated Show resolved Hide resolved
fmvilas
fmvilas previously approved these changes Dec 1, 2022
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

That's looking great mate 👏

@fmvilas
Copy link
Member

fmvilas commented Dec 1, 2022

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

@derberg
Copy link
Member

derberg commented Dec 1, 2022

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
Copy link
Collaborator Author

VisualBean commented Dec 1, 2022

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.

dalelane
dalelane previously approved these changes Dec 1, 2022
Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

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

I'm not familiar with Pulsar, so this isn't a super-informed review - but as a bindings doc it looks fine to me 👍

pulsar/README.md Outdated Show resolved Hide resolved
pulsar/README.md Outdated Show resolved Hide resolved
Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

Schemas seems to match the descriptions 👍

@VisualBean
Copy link
Collaborator Author

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
Copy link
Member

fmvilas commented Dec 7, 2022

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

@fmvilas
Copy link
Member

fmvilas commented Dec 9, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 44f7b58 into asyncapi:master Dec 9, 2022
@fmvilas
Copy link
Member

fmvilas commented Dec 9, 2022

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 :)

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

Successfully merging this pull request may close these issues.

Pulsar bindings for AsyncAPI
9 participants