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!: move operations to its own root object #806

Merged
merged 10 commits into from
Sep 26, 2022

Conversation

fmvilas
Copy link
Member

@fmvilas fmvilas commented Jun 13, 2022


title: "Move operations to its own root object"


Related issue(s):

#618 #663


@fmvilas fmvilas added the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label Jun 13, 2022
@fmvilas fmvilas force-pushed the add-operations-object branch 2 times, most recently from 04abcc2 to a5ac261 Compare June 13, 2022 14:13
@fmvilas fmvilas removed the 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) label Jun 13, 2022
spec/asyncapi.md Outdated Show resolved Hide resolved
spec/asyncapi.md Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@fmvilas
Copy link
Member Author

fmvilas commented Sep 6, 2022

/progress 60

New operations object is defined but there's still a pending discussion about how to reference the channel, i.e., via ID or $ref object. Read more on #663.

@derberg
Copy link
Member

derberg commented Sep 7, 2022

/progress 20

did it work :trollface:? or did you make sure that only your "progress comments" are taken into account? 😄

and regarding PR:

  • please add new object to ToC of the spec
  • it is a new root object, so should be added to AsyncAPI Object
  • I'm definitely up with using $ref for channel reference, let's be consistent everywhere though
  • I'm guessing the message goes away from operation as it stays in the channel, right? and it is handled here feat: add new channels object #827 so all good from my side

New operations object is defined but there's still a pending discussion about how to reference the channel, i.e., via ID or $ref object. Read more on #663.

@fmvilas I think we just need @dalelane view on that, or are you waiting for someone else?

@fmvilas
Copy link
Member Author

fmvilas commented Sep 9, 2022

/progress 60

did it work :trollface:? or did you make sure that only your "progress comments" are taken into account? 😄

It did work because it's meant to be used as a team so I just have to rely on the hope that you would not troll me so much 😅 (damn, as I'm writing this I'm already realizing I'm dead 😂)

I think we just need @dalelane view on that, or are you waiting for someone else?

Dale's point of view for sure. Would also love to have the opinions of —at least— @smoya, @char0n, and @magicmatatjahu. And of course, anyone else reading this is also welcome. I know you know but just so everyone knows :)


Regarding the review, thanks! I'll address all those points 👍

@fmvilas
Copy link
Member Author

fmvilas commented Sep 9, 2022

Alright, I think I addressed all the points @derberg. Would you please review it again?

"onUserSignUp": {
"summary": "Action to sign a user up.",
"description": "A longer description",
"channel": {
Copy link
Member

@smoya smoya Sep 9, 2022

Choose a reason for hiding this comment

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

I'm thinking that it might make sense to convert this into an array of channels (of references to channel) instead.
The use case that came to my mind was the fanout pattern, sending the same message to different channels.

For example, a user sign up action on a server might result in the server sending the same message to several queues on an EDA. Let's say those queues are owned by different departments, teams, or whatever.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK, the fanout pattern is usually implemented by the broker, not the application. I mean, for instance, RabbitMQ has the fanout exchange that lets you connect various queues to a specific routingKey. Kafka, NATS, and others have consumer groups, with which you can implement the fanout pattern. The application sends a message to a particular channel and then it gets distributed to many other queues or consumers. Am I missing something?

Now, I see the value it can have by allowing us to group multiple operations into one. Batch operations if you want. We just need to be very clear that if a send operation is performed against 1 out of 3 channels and it fails, it shouldn't stop the code from sending the other 2. Aren't we getting into the land of workflow definition already? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK, the fanout pattern is usually implemented by the broker, not the application.

This statement doesn't reflect the reality out there. The fanout could be perfectly implemented by the application; writing the same message to different Kafka topics is a completely valid use case. For logistics, you might want to set up different retention policies, permissions, etc to each topic.

Also, we can still use an AsyncAPI document to describe a message broker (wich turns to be the application), where this pattern is way more common.
At the end, a client should know where to connect to and to which channels a particular message will be sent so it can subscribe to one of those (depending on each scenario).

Aren't we getting into the land of workflow definition already?

TBH I don't think we are that far. This is still referring to an application definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's my point. I wouldn't say it's to support fanout but to batch operations. That's a pretty valid and common use case.

@derberg @dalelane @char0n @magicmatatjahu what do you folks think?

Copy link
Member

@smoya smoya Sep 9, 2022

Choose a reason for hiding this comment

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

I don't get why it's not to support the fanout pattern but batching/grouping operations. Messages are sent in parallel to different channels.

From Wikipedia:

In message-oriented middleware solutions, fan-out is a messaging pattern used to model an information exchange that implies the delivery (or spreading) of a message to one or multiple destinations possibly in parallel, and not halting the process that executes the messaging to wait for any response to that message.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only use case that comes to my mind right now is auditing. Some messages will be sent to their corresponding channel + the auditing channel. But I've never seen this done in the application code. It's always configured as a rule in the broker so you don't forget to audit anything.

Copy link
Member

@smoya smoya Sep 14, 2022

Choose a reason for hiding this comment

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

The use case I had in my mind is illustrated in the following pic grabbed from Salesforce blog. In particular, I'm talking about Point-to-Point architecture (even though the name is not correct IMHO) and not entering into which one is more scalable or less because it's not the main topic being discussed here.

I still believe this is part of the application definition and still not part of defining any message flow between apps.

Copy link
Member

Choose a reason for hiding this comment

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

but point to point is actually in contrast to EDA, right?

Copy link
Member

Choose a reason for hiding this comment

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

but point to point is actually in contrast to EDA, right?

The name on the picture is not correct as I mentioned in my previous comment. The right name I'm looking for is EventDriven Integration, and this article from Solace explains what it is: https://solace.com/blog/event-driven-architecture-difference-event-driven-integration

Copy link
Member Author

Choose a reason for hiding this comment

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

@smoya It looks like this requires its own discussion. Would you mind creating a follow-up issue? It would be great to find actual cases where this is required. Salesforce is great but I don't think it's a common use case.

@fmvilas fmvilas requested a review from jonaslagoni September 12, 2022 10:33
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

just to be clear, after we merge it, a release candidate will be released and published on the website

spec/asyncapi.md Outdated Show resolved Hide resolved
@fmvilas
Copy link
Member Author

fmvilas commented Sep 12, 2022

just to be clear, after we merge it, a release candidate will be released and published on the website

🤔 Should we avoid it for now? I just want to make progress towards 3.0.0 but we're still far. Should I use chore in this case?

@derberg
Copy link
Member

derberg commented Sep 12, 2022

tbh, I'm pro-pre-releases 😄 as they raise awareness. They will be published to asyncapi.com and will make it pretty clear to users going to "docs" that 3.0 is on the horizon. Docs is the most visited part of the website. We could add some big note at the beginning of the spec file, with some essential links on how to engage in works towards 3.0, just a thought.

@fmvilas
Copy link
Member Author

fmvilas commented Sep 12, 2022

We could add some big note at the beginning of the spec file, with some essential links on how to engage in works towards 3.0, just a thought.

That's a good idea. Will add it.

@fmvilas
Copy link
Member Author

fmvilas commented Sep 12, 2022

@derberg done. Let me know what you think.

spec/asyncapi.md Outdated Show resolved Hide resolved
Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

Niiice 🔥

@derberg
Copy link
Member

derberg commented Sep 21, 2022

@fmvilas

@fmvilas fmvilas force-pushed the add-operations-object branch from 84ad14d to 9bacca4 Compare September 22, 2022 15:05
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@fmvilas
Copy link
Member Author

fmvilas commented Sep 23, 2022

@dalelane mind having a look?

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.

LGTM 👍

@fmvilas
Copy link
Member Author

fmvilas commented Sep 26, 2022

/rtm

@asyncapi-bot asyncapi-bot merged commit 1d50ccb into asyncapi:next-major-spec Sep 26, 2022
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.0-next-major-spec.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

6 participants