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

[orc8r][service registry] Add proto for service registry service #2333

Merged
merged 2 commits into from Aug 17, 2020

Conversation

mpgermano
Copy link

Signed-off-by: mgermano mgermano@fb.com

Summary

The new service registry service will be used to provide registry
functionality for differing deployment environments (docker in dev,
k8s in prod). This PR adds the proto for the service.

Test Plan

n/a

Signed-off-by: mgermano <mgermano@fb.com>
// The ServiceRegistry interface implements a set of functionalities that can
// be used for service discovery.
// --------------------------------------------------------------------------
service ServiceRegistry {
Copy link
Contributor

Choose a reason for hiding this comment

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

Preference for the Uber style [0] where every request/response message is named directly after the RPC -- e.g. rpc ListAllServices (ListAllServicesRequest) returns (ListAllServicesResponse) {}

[0] https://github.com/uber/prototool/blob/dev/style/README.md#servicesrpcs

}

message AnnotationList {
repeated string annotations = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just return the flat annotation (i.e. not repeated), and the client side can handle optionally parsing into separate values

Also may be useful to name it annotation_value to disambiguate annotation (name of annotation) vs annotation_value (value of annotation)


message AnnotationList {
repeated string annotations = 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: editor config to always add eof newline

option go_package = "magma/orc8r/lib/go/protos";


// --------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Preference for following Go conventions when providing doc comments -- e.g.

// ServiceRegistry supports dynamic service discovery.

Signed-off-by: mgermano <mgermano@fb.com>
Copy link
Contributor

@hcgatewood hcgatewood left a comment

Choose a reason for hiding this comment

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

Lgtm

service ServiceRegistry {

// ListAllServices returns the service name of all services in the registry.
rpc ListAllServices (Void) returns (ListAllServicesResponse) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

In general I still prefer e.g. an empty ListAllServicesRequest defn over Void, though for this and other orc8r-internal grpc servicers it's not super relevant

@mpgermano mpgermano merged commit a9f6557 into magma:master Aug 17, 2020
@mpgermano mpgermano deleted the service_registry_service branch October 22, 2020 02:43
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.

None yet

2 participants