transport/grpcframer: create grpcframer package #7397
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7397 +/- ##
========================================
Coverage 81.42% 81.42%
========================================
Files 348 352 +4
Lines 26744 26917 +173
========================================
+ Hits 21775 21916 +141
- Misses 3779 3810 +31
- Partials 1190 1191 +1
|
|
And |
|
Also, i went ahead and marked some comments as resolved to reduce the noise in the review. |
| // Package grpchttp2 defines HTTP/2 types and a framer API and implementation. | ||
| package grpchttp2 | ||
|
|
||
| import "golang.org/x/net/http2/hpack" |
There was a problem hiding this comment.
This is a bummer. We cannot fully get rid of x/net/http2 to keep hpack
There was a problem hiding this comment.
That's true. But Ricardo did mention this in his design, and also x/net/http2/hpack is a separate package and I would assume this dependency would be much smaller than x/net/http2.
| // mem.BufferSlice. | ||
| WriteData(streamID uint32, endStream bool, data ...[]byte) error | ||
| // WriteData writes an HTTP/2 HEADERS frame to the stream. | ||
| // TODO: Once the mem package gets merged, headerBlock will change type to |
There was a problem hiding this comment.
need to double checl, but TODOs without ldap (TODO(arvindbright)) or without a bug TODO(b/123456) will be tagged by the internal golinter during import
There was a problem hiding this comment.
That linter check might not block the import I feel. We have a lot of TODOs which don't have either. And adding bug numbers here will not make sense. So, we can either add username, or leave it as is.
|
@zasweq : You seem to have requested changes on this PR. Do you mind taking a pass? Thanks. |
Create grpcframer package with type definitions.
Fixes: #7376
As part of the effort to reduce buffer copies in the transport layer of gRPC-Go, this PR declares the new API along with the necessary methods to implement a new HTTP/2 Framer.
RELEASE NOTES: none