-
Notifications
You must be signed in to change notification settings - Fork 86
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
Basic CacheProxy Server #6430
Basic CacheProxy Server #6430
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks pretty good, just a handful of nits
if err != nil { | ||
return err | ||
} | ||
stream.Send(rsp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we ignoring the error returned by stream.Send?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, fixed
} | ||
|
||
func (s *ByteStreamServerProxy) Write(stream bspb.ByteStream_WriteServer) error { | ||
remote_stream, err := s.remote_cache.Write(stream.Context()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/remote_stream/remoteStream/g
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
local_cache interfaces.Cache | ||
remote_cache repb.ActionCacheClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: camelCase var names instead of c_style
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(here, and throughout)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think i got 'em all
listen = flag.String("listen", "0.0.0.0", "The interface to listen on (default: 0.0.0.0)") | ||
port = flag.Int("port", 8080, "The port to listen for HTTP traffic on") | ||
sslPort = flag.Int("ssl_port", 8081, "The port to listen for HTTPS traffic on") | ||
internalHTTPPort = flag.Int("internal_http_port", 0, "The port to listen for internal HTTP traffic") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dumb question: what is this used for? Could we remove it and the whole block where we use it below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
if err != nil { | ||
return err | ||
} | ||
stream.Send(rsp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignored error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
if err != nil { | ||
return err | ||
} | ||
env.SetGRPCSServer(grpcsServer) | ||
return nil | ||
} | ||
|
||
func RegisterInternalGRPCServer(env *real_environment.RealEnv, regServices RegisterServices) error { | ||
func RegisterInternalGRPCServer(env *real_environment.RealEnv, config GRPCServerConfig, regServices RegisterServices) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find these Register functions that take a closure really confusing. Not your fault obvi :) They are not very idiomatic and are hard to use.
Maybe we can find a way to remove them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#6442 (i'll rebase this on that if it gets in relatively quickly)
9f7f544
to
f99992c
Compare
This PR introduces a new cache_proxy server (
//enterprise/server/cmd/cache_proxy
) that proxies the ActionCache, ByteStream, Capabilities, and ContentAddressableStorage services to the specified backend server. It authenticates with the backing cache by forwarding the provided API Key along. This server is currently running in dev atgrpcs://proxy.buildbudd.dev
, you can build against it by running something like:Next steps are to add a local cache, better auth, and then try routing executor-to-app traffic through the proxy.
I went back and forth over whether to switch the gRPC server configuration to use the Builder pattern instead of just adding a new configuration option, but chose the latter because it's a little simpler.
Related issues: N/A