Skip to content

Conversation

@printchard
Copy link
Contributor

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

@codecov
Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 10.00000% with 36 lines in your changes missing coverage. Please review.

Project coverage is 81.42%. Comparing base (bdd707e) to head (e450516).
Report is 19 commits behind head on master.

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     
Files Coverage Δ
internal/transport/grpchttp2/errors.go 100.00% <100.00%> (ø)
internal/transport/grpchttp2/framer.go 0.00% <0.00%> (ø)

... and 40 files with indirect coverage changes

@printchard printchard marked this pull request as ready for review July 8, 2024 23:38
@arvindbr8 arvindbr8 self-assigned this Jul 8, 2024
@arvindbr8 arvindbr8 self-requested a review July 8, 2024 23:38
@arvindbr8 arvindbr8 added the Type: Feature New features or improvements in behavior label Jul 8, 2024
@aranjans aranjans added this to the 1.66 Release milestone Jul 9, 2024
@easwars easwars assigned printchard and unassigned arvindbr8 Jul 9, 2024
@easwars
Copy link
Contributor

easwars commented Jul 9, 2024

And vet-proto is failing because we made a recent change to the codegen. So, it might be worth rebasing to master.

Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of style comments. I have some questions about implementation/where things live too.

Copy link
Contributor Author

@printchard printchard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you all for your feedback! I have addressed many of the issues pointed out, and I've left my contribution on some others.

@printchard printchard requested a review from easwars July 17, 2024 20:36
@printchard printchard assigned easwars and arvindbr8 and unassigned printchard Jul 17, 2024
@easwars easwars assigned printchard and unassigned easwars and arvindbr8 Jul 17, 2024
@printchard printchard requested a review from easwars July 17, 2024 21:31
// Package grpchttp2 defines HTTP/2 types and a framer API and implementation.
package grpchttp2

import "golang.org/x/net/http2/hpack"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bummer. We cannot fully get rid of x/net/http2 to keep hpack

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@printchard printchard requested review from arvindbr8 and easwars July 18, 2024 17:41
@printchard printchard assigned easwars and unassigned printchard Jul 18, 2024
// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great job getting this through!

@easwars
Copy link
Contributor

easwars commented Jul 18, 2024

@zasweq : You seem to have requested changes on this PR. Do you mind taking a pass? Thanks.

@easwars easwars assigned zasweq and unassigned easwars Jul 18, 2024
@arvindbr8 arvindbr8 merged commit 0231b0d into grpc:master Jul 23, 2024
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Type: Feature New features or improvements in behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement Custom Types for gRPC-Go Framer

5 participants