Add compressed reads to cas.go. #237
Conversation
rubensf
force-pushed
the
read-compression
branch
2 times, most recently
from
d9533be to
45fe3a3
Compare
go/pkg/client/cas.go
Outdated
| func (c *Client) maybeCompressReadBlob(hash string, sizeBytes int64, w io.WriteCloser) (string, io.WriteCloser, chan error, error) { | ||
| if c.CompressedBytestreamThreshold < 0 || int64(c.CompressedBytestreamThreshold) > sizeBytes { | ||
| // If we aren't compressing the data, theere's nothing to wait on. | ||
| dummyDone := make(chan error) |
There was a problem hiding this comment.
you can make the channel with a capacity of 1 so that writes don't block.
| } | ||
|
|
||
| type writerTracker struct { | ||
| io.Writer |
There was a problem hiding this comment.
Why not WriteCloser? Then you can pass Close as well?
There was a problem hiding this comment.
The arg passed in to readBlobStreamed is a io.Writer - if I make this a WriteCloser, I'll have to first wrap the given Writer.
go/pkg/client/cas.go
Outdated
| } | ||
| return n, nil | ||
|
|
||
| done := make(chan error) |
There was a problem hiding this comment.
Ditto, make chan of 1 so that writes don't block.
There was a problem hiding this comment.
I could - but I still have to launch a thread anyway to do de-compression on the side, so I gain no code simplicity but now have to allocate more data.
There was a problem hiding this comment.
The only thing gained is that the goroutine can exit sooner than it otherwise would, which I think is better. Up to you.
There was a problem hiding this comment.
Fair point - added it!
go/pkg/client/cas_test.go
Outdated
| offset int64 | ||
| limit int64 | ||
| want []byte // If nil, fake.blob is expected by default. | ||
| disableCompressionTest bool |
There was a problem hiding this comment.
you're not using this disableCompressionTest, right?
Although I'd rather add a bool compressed here, and set it explicitly in some cases.
There was a problem hiding this comment.
I was going to use it for the limit test cases below, then I realized I could do it differently -- removed now.
I also technically prefer using a "compressed" here, but I think we do can have a pragmatic check (that is, check whether limit is set) instead.
It follows the specs specified in bazelbuild/remote-apis#168, and it is similar to #232. Note that while the API still has room to change, it is mostly finalized and worth implementing. A caveat of this implementation is that while the `offset` in reads refers to the uncompressed bytes, the `limit` refers to the compressed bytes.
rubensf
force-pushed
the
read-compression
branch
from
45fe3a3 to
d883080
Compare
go/pkg/client/cas.go
Outdated
| } | ||
| return n, nil | ||
|
|
||
| done := make(chan error) |
There was a problem hiding this comment.
The only thing gained is that the goroutine can exit sooner than it otherwise would, which I think is better. Up to you.
| } | ||
|
|
||
| type writerTracker struct { | ||
| io.Writer |
It follows the specs specified in bazelbuild/remote-apis#168, and it is similar to bazelbuild#232. Note that while the API still has room to change, it is mostly finalized and worth implementing. A caveat of this implementation is that while the `offset` in reads refers to the uncompressed bytes, the `limit` refers to the compressed bytes.
It follows the specs specified in bazelbuild/remote-apis#168, and
it is similar to #232. Note that while the API still has room to
change, it is mostly finalized and worth implementing.
A caveat of this implementation is that while the
offsetin readsrefers to the uncompressed bytes, the
limitrefers to the compressedbytes.