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

Add RPC response message usage definition #158

Conversation

sophokles73
Copy link
Contributor

Addresses #112

@sophokles73 sophokles73 changed the title dd RPC response message usage definition Add RPC response message usage definition May 23, 2024
@sophokles73 sophokles73 added the documentation Improvements or additions to documentation label May 23, 2024
Copy link
Contributor

@stevenhartley stevenhartley left a comment

Choose a reason for hiding this comment

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

Minor enhancement only


== Service Interface Design

uProtocol uses https://protobuf.dev/programming-guides/proto3/#services[Protobuf] for defining a service provider's operations. Based on the error model defined above, the _Protobuf Message_ defined/used as the response of an operation *SHOULD* only contain the data/information that represents the outcome of _successful_ execution of the operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
uProtocol uses https://protobuf.dev/programming-guides/proto3/#services[Protobuf] for defining a service provider's operations. Based on the error model defined above, the _Protobuf Message_ defined/used as the response of an operation *SHOULD* only contain the data/information that represents the outcome of _successful_ execution of the operation.
uProtocol uses https://protobuf.dev/programming-guides/proto3/#services[Protobuf] for defining a service provider's operations. Based on the error model defined above, the _Protobuf Message_ defined/used as the response of an operation *SHOULD* only contain the data/information that represents the outcome of _successful_ execution of the operation.
If a response message is not required to be returned by the method that was invoked, the rpc **SHOULD** declare that it returns `google.protobuf.Empty` that would mean there is no `UMessage.payload`.
Example: `rpc DoSomething(DoSomethingRequest) returns (google.protobuf.Empty) {}`

Copy link
Contributor Author

@sophokles73 sophokles73 May 23, 2024

Choose a reason for hiding this comment

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

I have just read through Google's Protobuf API best practices and wonder, if we should instead suggest to define custom empty messages?

WDYT? @stevenhartley

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is better that we define an empty per service like below and not a generic Empty message but I think that is what you mean:

rpc DoSomething(DoSomethingRequest) returns DoSomethingResponse {}

message DoSomethingRequest {
  // A bunch of crap
}

message DoSomethingResponse {}

I would need to check what would then be in the UMessage.payload, a serialized empty message I guess (doesn't hurt)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is what I meant. IMHO this will also result in just an empty byte array, because the response message is defined exactly the same like google.protobuf.Empty.
I will update the spec accordingly ...

@sophokles73 sophokles73 marked this pull request as ready for review May 23, 2024 18:55
Copy link
Contributor

@stevenhartley stevenhartley left a comment

Choose a reason for hiding this comment

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

LGTM

@sophokles73
Copy link
Contributor Author

@stevenhartley do you also want me to change/update the core service protos accordingly, or do you want to do that in a separate PR? Technically, they should still be compliant with this updated spec, but IMHO we should change them nevertheless ...

@stevenhartley
Copy link
Contributor

@stevenhartley do you also want me to change/update the core service protos accordingly, or do you want to do that in a separate PR? Technically, they should still be compliant with this updated spec, but IMHO we should change them nevertheless ...

Separate PR is fine.

@stevenhartley stevenhartley merged commit 39d5934 into eclipse-uprotocol:main May 24, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants