[http] Initial codec splitting with test parametrization #10591
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,7 +11,9 @@ | |
| #include "common/http/header_utility.h" | ||
| #include "common/http/headers.h" | ||
| #include "common/http/http1/codec_impl.h" | ||
| #include "common/http/http1/codec_impl_legacy.h" | ||
| #include "common/http/http2/codec_impl.h" | ||
| #include "common/http/http2/codec_impl_legacy.h" | ||
| #include "common/http/path_utility.h" | ||
| #include "common/http/utility.h" | ||
| #include "common/network/utility.h" | ||
|
|
@@ -51,14 +53,26 @@ ServerConnectionPtr ConnectionManagerUtility::autoCreateCodec( | |
| headers_with_underscores_action) { | ||
| if (determineNextProtocol(connection, data) == Utility::AlpnNames::get().Http2) { | ||
| Http2::CodecStats& stats = Http2::CodecStats::atomicGet(http2_codec_stats, scope); | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior")) { | ||
| return std::make_unique<Http2::ServerConnectionImpl>( | ||
| connection, callbacks, stats, http2_options, max_request_headers_kb, | ||
| max_request_headers_count, headers_with_underscores_action); | ||
| } else { | ||
| return std::make_unique<Legacy::Http2::ServerConnectionImpl>( | ||
|
Contributor
There was a problem hiding this comment. Question - given the overhead we're going to have for edits during this period, would it make sense to tackle these one at a time? I don't know how long you estimate the work on each codec to be - if it's a week or so it's not worth splitting, but if it's going to be a month on HTTP/1 it'd be nice to not incur extra work on HTTP/2 during that time period. What are your thoughts here?
Contributor
Author
There was a problem hiding this comment. @yanavlasov Possibly. I only just finished merging edits in source files and tests for both codecs and parametrizing the H/1 codec unit test so it's easier to spot when it's out of sync. For H/2: It's very messy to parametrize the H/2 codec because it uses another test class. If we're okay not parametrizing the H/2 codec_impl_test and relying on just diffing the test files for the two H/2 codec implementation in fix_format (working on right now), then the H/2 work is done as well. Otherwise if we really want to parametrize the H/2 codec_impl test like I did for H/1 I'll split this out.
Contributor
There was a problem hiding this comment. I see. Yes parameterizing H/2 codec test would be quite a bit of surgery. I think it would be very good to have H/2 test parameterized and I'm going to see if it can be done reasonably easy. It is probably not a blocker for this PR.
Contributor
There was a problem hiding this comment. Thinking more about this, I think the way it is done now it totally fine for this PR. We have coverage for both H/2 codecs through separate tests. Yes, it is more overhead and a bit inconvenient. Note however that test files are exactly the same with the exception of the namespace declaration, so it should just be cut and paste of the test. |
||
| connection, callbacks, stats, http2_options, max_request_headers_kb, | ||
| max_request_headers_count, headers_with_underscores_action); | ||
| } | ||
| } else { | ||
| Http1::CodecStats& stats = Http1::CodecStats::atomicGet(http1_codec_stats, scope); | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_codec_behavior")) { | ||
| return std::make_unique<Http1::ServerConnectionImpl>( | ||
| connection, stats, callbacks, http1_settings, max_request_headers_kb, | ||
| max_request_headers_count, headers_with_underscores_action); | ||
| } else { | ||
| return std::make_unique<Legacy::Http1::ServerConnectionImpl>( | ||
| connection, stats, callbacks, http1_settings, max_request_headers_kb, | ||
| max_request_headers_count, headers_with_underscores_action); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.