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

Don't return rpc.Status as an explicit response data item #148

Open
avive opened this issue Jun 1, 2021 · 2 comments
Open

Don't return rpc.Status as an explicit response data item #148

avive opened this issue Jun 1, 2021 · 2 comments

Comments

@avive
Copy link
Contributor

avive commented Jun 1, 2021

All rpc api method calls started by clients return a status code and optional message.
I think that adding a status explicitly in responses is redundant and def not idiomatic grpc pattern.
e.g.:

message StopSmeshingResponse {   
   google.rpc.Status status = 1;
}

There are several other responses such as this in the smesher service.

So a method that just returns a status in its response should use the build-in status and status code.
See: https://grpc.io/docs/guides/error/

So a method that completes without an error and has not data to return such as StopSmeshingResponse should return status == 0 when there's no error or otherwise return one of the pre-defined error statuses and a message as defined here: https://grpc.github.io/grpc/core/md_doc_statuscodes.html - or an un-reserved code with message when one of the reserved error codes is not applicable.

@lrettig - please review.

So, StopSmeshingResponse should be defined like this:

message StopSmeshingResponse {   

}

The following status codes are never generated by the library and can be used:

  • INVALID_ARGUMENT
  • NOT_FOUND
  • ALREADY_EXISTS
  • FAILED_PRECONDITION
  • ABORTED
  • OUT_OF_RANGE
  • DATA_LOSS
@avive avive changed the title Don't return grpc status as repsonse members Don't return rpc.Status as an explicit response data items Jun 1, 2021
@avive avive changed the title Don't return rpc.Status as an explicit response data items Don't return rpc.Status as an explicit response data item Jun 1, 2021
@lrettig lrettig added good first issue Good for newcomers help wanted Extra attention is needed labels Jun 12, 2021
@lrettig
Copy link
Member

lrettig commented Jun 12, 2021

I think you're probably right and I think we should probably do this 👍

@avive
Copy link
Contributor Author

avive commented Jun 15, 2021

I've implemented this in the 0.3 branch

@avive avive removed good first issue Good for newcomers help wanted Extra attention is needed labels Jun 15, 2021
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

No branches or pull requests

2 participants