Differentiate Action Cache objects by instance name #148
Conversation
|
|
||
| return result, nil | ||
| } | ||
|
|
||
| func (s *grpcServer) maybeInline(inline bool, slice *[]byte, digest **pb.Digest, inlinedSoFar *int64) error { | ||
| func (s *grpcServer) maybeInline(instanceName string, inline bool, slice *[]byte, digest **pb.Digest, inlinedSoFar *int64) error { |
There was a problem hiding this comment.
I guess the instanceName arg is useless, since these items are stored in the CAS and this patch doesn't separate CAS entries by instance.
| if !s.cache.Contains(cache.CAS, instanceName, (*digest).Hash) { | ||
| err := s.cache.Put(cache.CAS, instanceName, (*digest).Hash, (*digest).SizeBytes, |
There was a problem hiding this comment.
May as well pass an empty string for the instance, I guess.
| @@ -90,8 +90,8 @@ func (s *grpcServer) BatchUpdateBlobs(ctx context.Context, | |||
| // Return the data for a blob, or an error. If the blob was not | |||
| // found, the returned error is errBlobNotFound. Only use this | |||
| // function when it's OK to buffer the entire blob in memory. | |||
| func (s *grpcServer) getBlobData(hash string, size int64) ([]byte, error) { | |||
| rdr, sizeBytes, err := s.cache.Get(cache.CAS, hash) | |||
| func (s *grpcServer) getBlobData(instanceName string, hash string, size int64) ([]byte, error) { | |||
There was a problem hiding this comment.
The instanceName arg seems to be useless here too, maybe just skip it and use the empty string?
| @@ -115,26 +115,26 @@ func (s *grpcServer) getBlobData(hash string, size int64) ([]byte, error) { | |||
| return data, rdr.Close() | |||
| } | |||
|
|
|||
| func (s *grpcServer) getBlobResponse(digest *pb.Digest) *pb.BatchReadBlobsResponse_Response { | |||
| func (s *grpcServer) getBlobResponse(instanceName string, digest *pb.Digest) *pb.BatchReadBlobsResponse_Response { | |||
| return nil | ||
| } | ||
|
|
||
| // Attempt to populate `resp`. Return errors for invalid requests, but | ||
| // otherwise attempt to return as many blobs as possible. | ||
| func (s *grpcServer) fillDirectories(resp *pb.GetTreeResponse, dir *pb.Directory, errorPrefix string) error { | ||
| func (s *grpcServer) fillDirectories(instanceName string, resp *pb.GetTreeResponse, dir *pb.Directory, errorPrefix string) error { |
| @@ -67,35 +68,36 @@ func NewHTTPCache(cache cache.Cache, accessLogger cache.Logger, errorLogger cach | |||
| } | |||
|
|
|||
| // Parse cache artifact information from the request URL | |||
There was a problem hiding this comment.
Since we'd be returning two different strings, I think we should document what they are here now.
|
Thanks for the PR! I will try to look through it properly in the next few days.
Can you clarify what you mean by a toolchain update that "breaks the remote caching", and describe the suggested workaround a bit? If you're referring to the fact that you get a bunch of cache misses after changing the toolchain, that is normal and expected. |
|
I don't have plans to work on this myself because I don't understand the use case well enough, and it could potentially be a breaking change for some users. If a toolchain is updated, shouldn't that change the corresponding Action messages, and therefore use different ActionCache lookup hashes? |
|
@mostynb the use case is pretty much just to work around bazelbuild/bazel#4558 without having to deploy multiple instances of bazel-remote. I imagine the behaviour change would be opt-in via a flag. |
|
So to summarise:
Is that right? If so, I think I can come up with a smaller opt-in feature to allow this. |
|
Exactly right, yes. |
Hello,
Every now and then a Clang update in our toolchain breaks the remote caching (usually fiffreneces in system headers like clang version numbers jumping from
7.0.0to7.1.0). A suggested work-around is to separate the caches.For http this is possible with
--remote_cache=http://localhost:8080/fooand for grpc by setting--remote_instance_name=bar.For now the instance name only affects the action cache because in my use case the CAS content cannot differ.
I'd appreciate some feedback. (I'm also a golang beginner - please bear with me)
Thanks,
Gregor
Fixes: #15