-
Notifications
You must be signed in to change notification settings - Fork 55
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
Guidelines for MQTT topics #2030
Guidelines for MQTT topics #2030
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if this PR is still in a draft status, I took the time to read it. Nice reflection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see as a missing requirement what is suggested by this comment #2030 (comment), i.e. the ability to use an MQTT bridge to connect as a child device a device that is running tedge.
About id uniqueness, my feeling is that we are making things more complex than they need and that the registration step is not required (at least to assign unique names). A line of thought is to take the same approach as for names of a file-system: file names are not attached to the files but to the directories and only file inodes have to be unique at the file-system level. If a child-device has 2 grand-child devices, it already has to distinguish them: somehow so the requirements of ids being namespaced are natural. The question could have been to enforce a notion of uniqueness for all recursively attached child devices and services (kind of inodes). But we seem to implicitly agree that we don't want that. It would be good to make it explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a slight preference for proposal 2 as proposal 1 also needs some kind of registration, i.e. add complexities without removing once and for all the main issue.
The goal was to support auto-registration for majority of the common use-cases involving direct child devices and services only. And the other key factor is observability as well, as I mentioned above. While looking at a trace of MQTT messages, it'd be far more easier to have some context instead of just random IDs. With just IDs, you always have to lookup into the inventory to differentiate what's what. May be I should list this observability at least as a nice-to-have requirement, if not must-have. |
Why random IDs? Proposal 2 can work with user-defined IDs. And yes observability is key but is not conflicting with the use of an inventory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can only approved this proposal ♥
- the proposal is crisp, sound and well explained
- there is clear separation of the user doc from the design rationale.
I still have some questions and I do think that what has been proposed for health checks must be fixed, but these can be addressed easily.
aee7a51
to
7a89942
Compare
Robot Results
Passed Tests
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved.
During implementation, any minor modifications will be made in the related PRs.
Proposed changes
Guidelines for MQTT topics
Types of changes
Paste Link to the issue
#2005
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments