feat(storage): Implement io.WriterTo in Reader #9659
Conversation
tritone
force-pushed
the
writerto-review
branch
2 times, most recently
from
6252744
to
1d1700d
Compare
tritone
added
the
kokoro:force-run
kokoro-team
removed
the
kokoro:force-run
tritone
changed the title
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
This allows the gRPC Reader to write directly into the application write buffer, saving a data copy. Users can get the benefit of this directly by explicitly calling Reader.WriteTo, but they can also benefit implicitly if they are calling io.Copy. A bunch of checksum logic had to be moved from the parent Reader into the transport Readers to make this work, since we need to update the checksum for every message read in WriteTo.
tritone
force-pushed
the
writerto-review
branch
from
1d1700d
to
b1620ca
Compare
| wantCRC uint32 | ||
| checkCRC bool | ||
| ) | ||
| if checksums := msg.GetObjectChecksums(); checksums != nil && checksums.Crc32C != nil && params.offset == 0 && params.length < 0 { |
There was a problem hiding this comment.
I don't have the full context here, but do you really want params.length to be negative here?
There was a problem hiding this comment.
I believe this is so that we only check the checksums on the entire object read, in which case params.length would be negative to indicate no limit.
There was a problem hiding this comment.
Yes, Brenna is correct.
storage/grpc_client.go
Outdated
| if err := r.runCRCCheck(); err != nil { | ||
| return 0, err | ||
| } | ||
| return 0, io.EOF |
There was a problem hiding this comment.
I noticed this above in the read too, but I wonder if these should be storage specific errors so users have more context? Or maybe wrapping happens at a higher layer. fmt.Errof("storage: something something: %w", io.EOF). Then your checks would need to be changed to errors.Is(err, io.EOF), but I think that change should happen regardless.
There was a problem hiding this comment.
Yeah I think there is still more we can do to have more coherently wrapped errors. it's been on the back burner.
| return 0, io.EOF | ||
| } | ||
|
|
||
| // No stream to read from, either never initialized or Close was called. |
There was a problem hiding this comment.
Is this handled in an above layer or is this documented as such on the public way this methods is called into?
There was a problem hiding this comment.
It's handled in an above layer; the stream is always initialized in NewRangeReader. Really the only way you can trigger this is as an end user is by calling Close and then trying to read more.
tritone
added
the
automerge
gcf-merge-on-green
bot
removed
the
automerge
🤖 I have created a release *beep* *boop* --- ## [1.40.0](https://togithub.com/googleapis/google-cloud-go/compare/storage/v1.39.1...storage/v1.40.0) (2024-03-29) ### Features * **storage:** Implement io.WriterTo in Reader ([#9659](https://togithub.com/googleapis/google-cloud-go/issues/9659)) ([8264a96](https://togithub.com/googleapis/google-cloud-go/commit/8264a962d1c21d52e8fca50af064c5535c3708d3)) * **storage:** New storage control client ([#9631](https://togithub.com/googleapis/google-cloud-go/issues/9631)) ([1f4d279](https://togithub.com/googleapis/google-cloud-go/commit/1f4d27957743878976d6b4549cc02a5bb894d330)) ### Bug Fixes * **storage:** Retry errors from last recv on uploads ([#9616](https://togithub.com/googleapis/google-cloud-go/issues/9616)) ([b6574aa](https://togithub.com/googleapis/google-cloud-go/commit/b6574aa42ebad0532c2749b6ece879b932f95cb9)) * **storage:** Update protobuf dep to v1.33.0 ([30b038d](https://togithub.com/googleapis/google-cloud-go/commit/30b038d8cac0b8cd5dd4761c87f3f298760dd33a)) ### Performance Improvements * **storage:** Remove protobuf's copy of data on unmarshalling ([#9526](https://togithub.com/googleapis/google-cloud-go/issues/9526)) ([81281c0](https://togithub.com/googleapis/google-cloud-go/commit/81281c04e503fd83301baf88cc352c77f5d476ca)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
This allows the gRPC Reader to write directly into the application write buffer, saving a data copy.
Users can get the benefit of this directly by explicitly calling Reader.WriteTo, but they can also benefit implicitly if they are calling io.Copy.
A bunch of checksum logic had to be moved from the parent Reader into the transport Readers to make this work, since we need to update the checksum for every message read in WriteTo.