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

Avoid sending the node id in the Attach rpc #58

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

rgallor
Copy link
Collaborator

@rgallor rgallor commented Aug 27, 2024

Avoid sending the node id in the attach rpc since it is already inserted in grpc metadata of every rpc the node send.

@@ -23,6 +23,11 @@ syntax = "proto3";

package astarteplatform.msghub;

/* This message defines the introspection a node should send when attaching to the Astarte message hub. */
message Introspection {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Introspection and InterfacesJson messages might be confusing as the difference between them is unclear.
If the content is the same, why not use InterfacesJson in the Attach method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, I had the same thought, still I decided to differentiate the 2 messages since Introspection represents the whole set of interfaces transmitted during the attach procedure, whereas InterfacesJson may represent a subset of them or new interfaces to be added to the Node interfaces. Nevertheless, since both the 2 messages represent a set of interfaces, it seems reasonable to use only one of them (the most generic one, thus InterfacesJson).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@harlem88 @joshuachp what's your opinion?

Copy link
Contributor

@joshuachp joshuachp Aug 27, 2024

Choose a reason for hiding this comment

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

I think that having two structures would be better in this case, so in the future we can add additional fields to the rcp Attach message (now called Introspection) and still be backwards compatible.

Perhaps the name Introspection is confusing in this case, I would keep the Node message or a rename it to something related to the Attach call, and with a field called introspection inside.

If we keep the Node message, it will be a smaller change since we only have to remove the UUID field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, probably maintaining the Node parameter in the Attach rpc (with only the introspection data) is a good option, also for backward compatibility

Makefile Outdated Show resolved Hide resolved
@rgallor rgallor requested a review from sorru94 August 27, 2024 12:56
Avoid sending the node id in the attach rpc since it is already
inserted in grpc metadata of every rpc the node send.
Also, remove generated .py files when executing `make install`.

Signed-off-by: Riccardo Gallo <riccardo.gallo@secomind.com>
@sorru94 sorru94 merged commit 06a6c08 into astarte-platform:master Aug 27, 2024
13 checks passed
@rgallor rgallor deleted the refactor/node-id branch August 30, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants