-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
transport/grpchttp2: add http2.Framer bridge #7453
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7453 +/- ##
==========================================
+ Coverage 81.41% 81.67% +0.26%
==========================================
Files 357 359 +2
Lines 27261 27515 +254
==========================================
+ Hits 22195 22474 +279
+ Misses 3843 3817 -26
- Partials 1223 1224 +1
|
d8f309d
to
f43d3b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet to completely review the tests. But this should unblock you for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I have replied to the conversations, I'll make the code changes to the rest later today.
Feels like we are very close to the finish line on this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be done in this last pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, only a few optional comments. I dont want to block merge on them. I would wait for an approval from @easwars
}, nil | ||
default: | ||
buf := fr.pool.Get(int(hdr.Size)) | ||
huf := f.(*http2.UnknownFrame) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, what's a huf
? lol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant to type uf
lol.
return uint16(b[0])<<8 | uint16(b[1]) | ||
} | ||
|
||
// checkWrittenHeader takes a byte buffer representing a written frame header |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name has changed. Needs fixing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the actual description but forgot the name 😅. It is done now.
Create HTTP2Bridge on the grpchttp2 package.
As part of the ongoing effort to migrate into a new Framer inside the grpchttp2 package, I added an adapter for the http2.Framer implementation to provide a way to opt-out of the new feature.
RELEASE NOTES: none