-
-
Notifications
You must be signed in to change notification settings - Fork 369
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
QPACK and naive H3 implementation #136
Conversation
c00f5ff
to
db2d23a
Compare
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 60.03% 66.68% +6.65%
==========================================
Files 21 32 +11
Lines 5274 6495 +1221
==========================================
+ Hits 3166 4331 +1165
- Misses 2108 2164 +56
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
==========================================
+ Coverage 74.82% 77.66% +2.83%
==========================================
Files 24 41 +17
Lines 6150 9197 +3047
==========================================
+ Hits 4602 7143 +2541
- Misses 1548 2054 +506
Continue to review full report at Codecov.
|
cf0e752
to
bc18037
Compare
c9871fe
to
92bbc6d
Compare
874246b
to
00d5c95
Compare
I think we are near the point where it would be nice to plan merging this. It has been tested against lightspeed's endpoint and there hasn't been necessary modification in the qpack code. I just had to use what's in #130 to achieve this. For the moment, encoder stream hasn't proved to be totally functional, but I'll soon get it tested. It's now big enough for me to worry about the review, and willing to work with smaller iterations (surely is a beginner thing). |
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.
There are a few usages of allow(dead_code) in here, why are they needed? Did you investigate reusing parts of HPACK from the h2 implementation?
@djc I did not think of that. I'm not sure the common code surface would be that big. Maybe for the primitive types like |
I think that's what I was looking at, but I understand if there's not enough overlap. 👍 |
I think we're ready to merge ! The interop tool has been updated with only a very simple H3 scenario (with no encoder stream, nor receiving anything from server other than the response). Trying to implement a full-featured client, I started to think it was too much code to write in form of a quick implementation. So it would be better to continue writing a solid code into another 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.
So looking at the interop tool additions mainly for now, this looks pretty good. @Ralith, do you want to review this more before it gets merged?
I'll try to have a look at the futuresy bits tomorrow per @stammw's request, at least. |
🎉 |
Continuing the work of @evolix1, from #20, which is blocking #130.
Features:
Optimization:
LiteralWithPostBaseNameRef::new(index, field.value.clone())
: No