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(adr): addition of north-south messaging ADR #656

Merged

Conversation

jpwhitemn
Copy link
Member

fix: #640
Signed-off-by: jpwhitemn jpwhite_mn@yahoo.com

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-docs/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

  • Changes have been rendered and validated locally using mkdocs-material (see edgex-docs README)

fix: edgexfoundry#640
Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
Copy link
Contributor

@TomBrennan-Eaton TomBrennan-Eaton left a comment

Choose a reason for hiding this comment

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

The proposal makes sense to me.
I do wonder about the mapping that the command service has to do, to transform the topics on request and on response; I think an example of the topic path on the northside used on request and response would be helpful, eg for the sensor01 example.
Is it the case that the request topic must include the device name and resource name, so the command service just has to map to the correct device service?
Would it make sense to echo the command name into the response, as a reality check?

@TomBrennan-Eaton
Copy link
Contributor

Is it acceptable for more than one response to be published by the device service on the same correlation ID? Eg, send back "Acknowledged", then "Scheduled", then "Starting", then "Done" statuses?

@TomBrennan-Eaton
Copy link
Contributor

Should probably state if the burden is on the northside to manage timeouts or resending if no response - ie, the command service is "best effort" and will not do these things.

docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
Copy link
Member

@tonyespy tonyespy left a comment

Choose a reason for hiding this comment

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

Please see comments inline.

One global question, will it be possible to send/receive binary data (e.g. CBOR) using this approach?

docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
addressed the internal/external message bus issue

cleaned up message envelope vs payload issue

Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
…ge comms section

Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
…er core working group meeting of 4/7/22

Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
iain-anderson
iain-anderson previously approved these changes Apr 8, 2022
lenny-goodell
lenny-goodell previously approved these changes Apr 12, 2022
bnevis-i
bnevis-i previously approved these changes Apr 21, 2022
Copy link
Collaborator

@bnevis-i bnevis-i left a comment

Choose a reason for hiding this comment

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

I believe due diligence has been done on this one. Let's send this up for a vote.

Copy link
Member

@tonyespy tonyespy left a comment

Choose a reason for hiding this comment

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

Looks much better, but I still have a few comments inline...

docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
docs_src/design/adr/0023-North-South-Messaging.md Outdated Show resolved Hide resolved
Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
…ions is still an open issue left for implementation

per TSC meeting of 4/27/22

Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
tonyespy
tonyespy previously approved these changes Apr 27, 2022
bnevis-i
bnevis-i previously approved these changes Apr 27, 2022
iain-anderson
iain-anderson previously approved these changes Apr 28, 2022
lenny-goodell
lenny-goodell previously approved these changes Apr 28, 2022
…pproved ADRs

Signed-off-by: jpwhitemn <jpwhite_mn@yahoo.com>
Copy link
Contributor

@JamesKButcher JamesKButcher left a comment

Choose a reason for hiding this comment

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

LGTM. As approved on Core WG calls

@jpwhitemn jpwhitemn merged commit 742dd97 into edgexfoundry:main Apr 29, 2022
edgex-jenkins added a commit that referenced this pull request Apr 29, 2022
…ge-bus

Signed-off-by: edgex-jenkins <collab-it+edgex@linuxfoundation.org>
@jpwhitemn jpwhitemn deleted the adr-0023-north-south-message-bus branch April 29, 2022 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADR Architecture Decision Record documentation kamakura
Projects
None yet
Development

Successfully merging this pull request may close these issues.

North-South message bus ADR
9 participants