feat: initial grpc support #552
Conversation
ybt195
force-pushed
the
grpc-server
branch
7 times, most recently
from
b33b588
to
3870658
Compare
.circleci/config.yml
Outdated
| @@ -115,6 +115,8 @@ commands: | |||
| - restore_cache: | |||
| keys: | |||
| - det-go-deps-v1dev6-{{ checksum "master/go.sum" }}-{{ checksum "agent/go.sum" }} | |||
| - run: curl -LO https://github.com/protocolbuffers/protobuf/releases/download/v3.12.1/protoc-3.12.1-linux-x86_64.zip | |||
There was a problem hiding this comment.
blocking: let's move this into it's own command (maybe install-protoc and have go-get-deps use that command before the restore_cache
master/proto/api/v1/agent.proto
Outdated
| enum SortBy { | ||
| // SORT_BY_UNSPECIFIED returns agents sorted by id. | ||
| SORT_BY_UNSPECIFIED = 0; | ||
| // SORT_BY_UNSPECIFIED returns agents sorted by id. |
There was a problem hiding this comment.
blocking: need to fix the comments here (copy paste errors)
master/proto/agent/v1/agent.proto
Outdated
| Container container = 4; | ||
| } | ||
|
|
||
| // Container is a docker container that is either scheduled to run or is currently running on a |
There was a problem hiding this comment.
non-blocking: docker -> Docker
| @@ -168,6 +169,14 @@ func (m *Master) startServers() error { | |||
| runServer := func(server *http.Server) { | |||
| errs <- errors.Wrap(m.echo.StartServer(server), servers[server]+" failed") | |||
| } | |||
| go func() { | |||
| if err := grpc.RegisterHTTPProxy(m.echo, m.config.GRPCPort); err != nil { | |||
There was a problem hiding this comment.
question: can we explain why we are doing something slightly different here than than the other ones?
As in, why don't we do errs <- grpc.RegisterHTTPProxy(m.echo, m.config.GRPCPort)?
There was a problem hiding this comment.
RegisterHTTPProxy does not block; it just adds the endpoints to the echo server. In other words it does not start its own server. All the other calls start a server and block until there is a fatal error.
master/internal/agent/agents.go
Outdated
| if msg.Label != "" && msg.Label != a.Label { | ||
| continue | ||
| } | ||
| response.Agents = append(response.Agents, toProtoAgent(a)) |
There was a problem hiding this comment.
non-blocking: curious why not do this the other way:
if msg.Label == "" || msg.Label == a.Label {
response.Agents = append(response.Agents, toProtoAgent(a))
}
I personally think this is a bit clearer, but it definitely is a personal preference.
master/internal/agent/agents.go
Outdated
| case proto.GetAgentsRequest_SORT_BY_UNSPECIFIED, proto.GetAgentsRequest_SORT_BY_ID: | ||
| fallthrough | ||
| default: | ||
| return a1.Id < a2.Id |
There was a problem hiding this comment.
question: curious why take this approach instead of using return a1.ID < a2.ID as part of the SORT_BY_ID block, and a panic here?
The current approach seems a tad dangerous. If we were to add another sort type and someone were to put it below SORT_BY_ID it wouldn't work correctly.
master/internal/agent/agents.go
Outdated
| return a1.Id < a2.Id | ||
| } | ||
| }) | ||
| if msg.OrderBy == proto.GetAgentsRequest_ORDER_BY_DESC { |
There was a problem hiding this comment.
question: why not make this part of the sorting function?
|
@ybt195 when we do ship this, we should make sure the team is aware of what needs to be done on their end to make sure it builds. |
master/proto/api/v1/agent.proto
Outdated
| enum OrderBy { | ||
| // ORDER_BY_DESC returns agents in ascending order. | ||
| // Returns agents in ascending order. |
There was a problem hiding this comment.
non-blocking: I'm not sure if it makes sense to explain the default behavior?
Someone may rely on this since we claim that this is what happens. Which may burn us if we change the default behavior.
* feat: support container run type enroot. * feat: support container run type enroot.
* feat: support container run type enroot. * feat: support container run type enroot.
* feat: support container run type enroot. * feat: support container run type enroot.
Description
Test Plan
Commentary (optional)