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: assign channels to servers #531

Merged
merged 34 commits into from
Sep 23, 2021

Conversation

GeraldLoeffler
Copy link
Contributor

@GeraldLoeffler GeraldLoeffler commented Apr 27, 2021


title: "Assign channels to servers through their names"

Add servers field to channel items, so that channels can be assigned to a sub-set of servers using the names of those servers. (A server's name is the string key used for defining that server in the top-level servers object.)

Solves the issues discussed in the related issues:


Related issue(s):

#244
#255
#475

@fmvilas fmvilas added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Apr 27, 2021
@smoya
Copy link
Member

smoya commented Apr 27, 2021

I like the approach of using labels here. In fact, It made me think about Kubernetes labels and selectors.
Perhaps, we could move one step further and add the same metadata concept for servers so labels can be a map of string => string as well. In this way we could also tag servers with labels such as env: production or similar.

@fmvilas
Copy link
Member

fmvilas commented Apr 28, 2021

@smoya I think going the same path as k8s is a bit of a premature optimization for now IMHO. AFAIK, nobody has asked for a more sophisticated mechanism, and adding it will introduce potentially unnecessary complexity to the spec.

@derberg
Copy link
Member

derberg commented Apr 28, 2021

@GeraldLoeffler could you clarify more about why it is Server and Channel relation and not Server and Message relation? There is a use case that one channel can have multiple messages on a given operation (messages.oneOf). You can have 2 servers with the same channel, but some messages are available only on 1 server. What would be the possible problem if we have serverLabel on the message and not the channel?

Another suggestion from my side would be a bit more modification on the example. Now labels kind of duplicate information like server key and protocol. This may cause confusion.

Another thinking exercise is: at first sight labels seem to be nifty solution but is it really necessary here, do we have a use case for so many servers that they need some kind of label? wouldn't it be better -> the channel/message just have a server object that is a list of servers? we would leverage $ref usage here of course? I think it would be more aligned with the spec 🤔 like that users could reuse existing solutions like $ref and traits to optimize AsyncAPI document and keep it DRY, instead of the label system.

Sorry for all these challenges 😅

Huge additional request, please explicitly mention this PR as a comment in all the linked issues so people that are interested the most are notified.

@GeraldLoeffler
Copy link
Contributor Author

GeraldLoeffler commented Apr 28, 2021

@GeraldLoeffler could you clarify more about why it is Server and Channel relation and not Server and Message relation? There is a use case that one channel can have multiple messages on a given operation (messages.oneOf). You can have 2 servers with the same channel, but some messages are available only on 1 server. What would be the possible problem if we have serverLabel on the message and not the channel?

it was simply a question of modelling the most obvious association, the one between server and channel:

  • a queue/topic (= channel) is inherently (often statically) bound to a message broker (= server)
  • a URL path (= channel) is inherently (typically statically) bound to a HTTP server (= server)

So far in the spec it was assumed that in the scope of a given AsyncAPI document, all channels are available on all servers. I'm trying to relax that requirement.

You are saying that the same channel may exist on several servers, but some messages may only be sent/received via that channel to/from a subset of those servers. True, that can happen too, but i didn't have the need to model that and so it's not in my proposal. If we wanted to include it we would need an optional serverLabels field on the message (in addition to the one on the channel).

@GeraldLoeffler
Copy link
Contributor Author

Another suggestion from my side would be a bit more modification on the example. Now labels kind of duplicate information like server key and protocol. This may cause confusion.

agreed, the examples could be better: happy to refine them once we have agreement on the approach.

@GeraldLoeffler
Copy link
Contributor Author

GeraldLoeffler commented Apr 28, 2021

Another thinking exercise is: at first sight labels seem to be nifty solution but is it really necessary here, do we have a use case for so many servers that they need some kind of label? wouldn't it be better -> the channel/message just have a server object that is a list of servers? we would leverage $ref usage here of course? I think it would be more aligned with the spec 🤔 like that users could reuse existing solutions like $ref and traits to optimize AsyncAPI document and keep it DRY, instead of the label system.

your proposal is to bind channels to servers by explicitly listing all servers to which a channel applies. This is certainly possible, but cumbersome in the typical case of having many environments. For example, for a protocol translator from mqtt to jms:

servers:
  mqtt-dev:
    url: ...
  mqtt-test:
    url: ...
  mqtt-prod:
    url: ...
  jms-dev:
    url: ...
  jms-test:
    url: ...
  jms-prod:
    url: ...

from-channel-1:
  servers:
    - $ref "...mqtt-dev"
    - $ref "...mqtt-test"
    - $ref "...mqtt-prod"
  ...
from-channel-2:
  servers:
    - $ref "...mqtt-dev"
    - $ref "...mqtt-test"
    - $ref "...mqtt-prod"
  ...
to-channel-1:
  servers:
    - $ref "...jms-dev"
    - $ref "...jms-test"
    - $ref "...jms-prod"
  ...
to-channel-2:
  servers:
    - $ref "...jms-dev"
    - $ref "...jms-test"
    - $ref "...jms-prod"
  ...

versus

servers:
  mqtt-dev:
    url: ...
    label: ["from"]
  mqtt-test:
    url: ...
    label: ["from"]
  mqtt-prod:
    url: ...
    label: ["from"]
  jms-dev:
    url: ...
    label: ["to"]
  jms-test:
    url: ...
    label: ["to"]
  jms-prod:
    url: ...
    label: ["to"]

from-channel-1:
  serverLabels: ["from"]
  ...
from-channel-2:
  serverLabels: ["from"]
  ...
to-channel-1:
  serverLabels: ["to"]
  ...
to-channel-2:
  serverLabels: ["to"]
  ...

I think this example shows the tradeoff between the two approaches.

@smoya
Copy link
Member

smoya commented Apr 28, 2021

@GeraldLoeffler could you clarify more about why it is Server and Channel relation and not Server and Message relation? There is a use case that one channel can have multiple messages on a given operation (messages.oneOf). You can have 2 servers with the same channel, but some messages are available only on 1 server. What would be the possible problem if we have serverLabel on the message and not the channel?

Would it make sense to add, instead, an extra link between Servers and Operations? This is something we have considered when designing the new Intent-driven API for the Parser(s). See https://github.com/asyncapi/parser-api/blob/master/docs/v1.md#operation.

However, having several resources linking to servers can introduce issues such as mutual exclusion. What if a channel specifies its relation to some servers but their operations are linked to others?

@GeraldLoeffler
Copy link
Contributor Author

Calling all interested parties to please support one of the following proposals on this topic:

  1. associate channels to servers via simple string labels on servers (original proposal by @GeraldLoeffler, also shown in the 2nd part here)
  2. ditto, but extend the labelling scheme to be name-value pairs exactly as in Kubernetes (proposal by @smoya)
  3. associate channels to servers by referring to the servers directly, without any labels (proposal by @derberg, shown in the 1st part here)

In addition, there were proposal to go beyond the association of channels to servers:

  1. also associate operations to servers (proposal by @smoya)
  2. also associate messages to servers (proposal by @derberg)

Proposals 4. and 5. are additional associations. So the mechanism (string labels, key-value-pair labels, or direct reference) to link to servers in 4. and 5. should match whatever is chosen to link channels to servers. I therefore propose to cover 4.-5. by a separate PR and focus here on 1.-3. .

Please vote on 1., 2., or 3., and also state your support for 4. and/or 5. (@smoya, @derberg, @fmvilas).

@tyazid
Copy link

tyazid commented May 1, 2021

@GeraldLoeffler could you clarify more about why it is Server and Channel relation and not Server and Message relation? There is a use case that one channel can have multiple messages on a given operation (messages.oneOf). You can have 2 servers with the same channel, but some messages are available only on 1 server. What would be the possible problem if we have serverLabel on the message and not the channel?

Would it make sense to add, instead, an extra link between Servers and Operations? This is something we have considered when designing the new Intent-driven API for the Parser(s). See https://github.com/asyncapi/parser-api/blob/master/docs/v1.md#operation.

However, having several resources linking to servers can introduce issues such as mutual exclusion. What if a channel specifies its relation to some servers but their operations are linked to others?

Rather consider the association of an operation with a server as optional because it is not the general case. Indeed, an operation should (and it is) be associated with a message which can be exchanged with one or several servers... in my opinion.

@DavidBiesack
Copy link

See also a similar request for OpenAPI to define reusable sets of parameters (or parameter sets). There may be some ideas presented there that you can use to refine this feature proposal. I.e. labels are juts one way to define a set of servers (all servers sharing a label constitute a server set), and you want to be able to apply or associate a channel to one or more server sets.

@derberg
Copy link
Member

derberg commented May 4, 2021

@GeraldLoeffler I think it is hard to perform any voting if there are still some unknowns.

agreed, the examples could be better: happy to refine them once we have agreement on the approach.

I think good example is necessary for others to follow proposal

your proposal is to bind channels to servers by explicitly listing all servers to which a channel applies. This is certainly possible, but cumbersome in the typical case of having many environments.

The advantage is that you have validation out of the box, parsing fails if you want to parse outdated reference. With labeling, one can change a label on a channel to a name that is not on the server list. Of course this could also be validated but therefore needs to be specified in the spec so parser know it is mandatory to implement.


Can you provide a full example of AsyncAPI file with thes new properties. I'd like to have a closer look on a real use case and see how it works for websocket. We have over representation of people supporting concepts with broker in the middle and I just want to explore how this would work for websocket, if this is of any use actually.

also associate messages to servers (proposal by @derberg)

Let's drop it. I see this in websockets only and for the server code generation where in fact in my opinion this should be really solved by having 2 separate spec documents, with external common parts

@GeraldLoeffler
Copy link
Contributor Author

@derberg @smoya , @fmvilas : going the path of least resistence here and adopting @derberg's proposal, i.e., associating channels to servers by referring to the servers directly, without any labels. This works and is the simplest possible solution.

Have also added a (i think) convincing example:

servers:
  webSocketsDev:
    $ref: "#/components/servers/wsDev"
  webSocketsProd:
    $ref: "#/components/servers/wsProd"
  amqpProd:
    $ref: "#/components/servers/rabbitmqBrokerInProd"
  amqpStage:
    $ref: "#/components/servers/rabbitmqBrokerInStaging"

webUICommandsQueue:
  description: This application publishes WebUICommand messages to this AMQP queue which it had received via WebSockets on another channel
  servers:
    - $ref: "#/components/servers/rabbitmqBrokerInProd"
    - $ref: "#/components/servers/rabbitmqBrokerInStaging"
  subscribe:
    message:
      $ref: "#/components/messages/WebUICommand"
  bindings:
    amqp:
      is: queue
...

components:
  servers:
    rabbitmqBrokerInProd:
      url: staging.gigantic-server.com
      description: Staging server
      protocol: amqp
      protocolVersion: 0.9.1
   rabbitmqBrokerInStaging
      url: api.gigantic-server.com
      description: Production server
      protocol: amqp
      protocolVersion: 0.9.1
    wsDev:
      ...
    wsProd:
      ...

Please review https://github.com/asyncapi/spec/pull/531/files and approve.

@GeraldLoeffler GeraldLoeffler changed the base branch from master to 2021-09-release September 21, 2021 12:15
@GeraldLoeffler
Copy link
Contributor Author

@derberg : rebased to release branch - done

@derberg derberg added 🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md) and removed keep-open Prevents stale bot from closing this issue or PR 💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md) labels Sep 23, 2021
spec/asyncapi.md Outdated Show resolved Hide resolved
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.

A reference implementation in JSON Schema and Parser are already released in individual release candidates. I changed this proposal to Stage 3.

There is one error IMHO here in this PR, please have a look at the individual comment to markdown file cc @fmvilas @magicmatatjahu

@derberg
Copy link
Member

derberg commented Sep 23, 2021

@GeraldLoeffler fyi we cannot make fixes on our own as you did PR from some kind of org-level fork and we do not have rights as maintainer to push to this PR.

Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@sonarcloud
Copy link

sonarcloud bot commented Sep 23, 2021

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
No Duplication information No Duplication information

@GeraldLoeffler
Copy link
Contributor Author

@GeraldLoeffler fyi we cannot make fixes on our own as you did PR from some kind of org-level fork and we do not have rights as maintainer to push to this PR.

i accepted your suggestion and gave you write access to my repo.

@derberg
Copy link
Member

derberg commented Sep 23, 2021

@GeraldLoeffler perfect, the only part that is missing is updated description that reflects what is actually implemented

@GeraldLoeffler
Copy link
Contributor Author

@GeraldLoeffler perfect, the only part that is missing is updated description that reflects what is actually implemented

done

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.

👏🏼

@derberg derberg merged commit 8f5ac24 into asyncapi:2021-09-release Sep 23, 2021
@derberg derberg deleted the channels-to-servers branch September 23, 2021 15:01
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.2.0-2021-09-release.4 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@fmvilas
Copy link
Member

fmvilas commented Sep 23, 2021

Super happy to see this merged 👏

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.2.0 🎉

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.

8 participants