make http/2 settings configurable (#988) by jwfang · Pull Request #989 · envoyproxy/envoy

jwfang · 2017-05-18T12:40:09Z

for #988

WIP, for your preview

jwfang · 2017-05-18T12:51:26Z

i will rename CodecOptions to Http2Options,
and the old codec_options will keep its name.

seems this is only used by http2, should we move it to Http2 namespace ?

mattklein123

In general this is the right approach. Will need tests, docs, etc.

@PiotrSikora can you potentially help shepherd this change through while I'm on vacation. cc @htuch

mattklein123 · 2017-05-18T17:14:05Z

 * A list of options that can be specified when creating a codec.
 */
-class CodecOptions {
+class CodecOptionFlags {


This whole thing is a mess and will be cleaned up as part of the v2 API effort. cc @htuch @PiotrSikora

In the interim, I think it's OK to leave this in the http namespace.

mattklein123 · 2017-05-18T17:14:33Z

 };

+struct CodecOptions {
+  uint64_t flag_options;


nit: all members should end with '_'

mattklein123 · 2017-05-18T17:15:51Z

-class CodecOptions {
+class CodecOptionFlags {
 public:
  static const uint64_t NoCompression = 0x1;


It's not part of your change, but as long as you are in here, can you please rename NoCompression to DisableDynamicHPACKTable. Don't bother changing the actual JSON config. We will fix that in v2 API.

mattklein123 · 2017-05-18T17:16:34Z

   *         @see Http::CodecOptions.
   */
-  virtual uint64_t httpCodecOptions() const PURE;
+  virtual Http::CodecOptions httpCodecOptions() const PURE;


return const Http::CodecOptions& and fix doc comment above.

mattklein123 · 2017-05-18T17:17:08Z

+
+  if (codec_options.flag_options & CodecOptionFlags::NoCompression) {
    iv.push_back({NGHTTP2_SETTINGS_HEADER_TABLE_SIZE, 0});
    conn_log_debug("disabling header compression", connection_);


can you fix this comment to to read: "setting HPACK dynamic table size to 0"

1. change original uint32_t CodecOptions to Http2Settings, and move CodecOptions into Http2Settings; 2. default ctor for Http2Settings, and move default consts into Http2Settings; 3. hard code maxConcurrentStreams return temporarily till PR #981.

//test/integration:http2_integration_test FAILED in 1 out of 2 in 0.2s /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/execroot/ci/bazel-out/local-dbg/testlogs/test/integration/http2_integration_test/test.log //test/integration:http2_upstream_integration_test FAILED in 1 out of 2 in 0.2s /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/execroot/ci/bazel-out/local-dbg/testlogs/test/integration/http2_upstream_integration_test/test.log //test/integration:ssl_integration_test FAILED in 1 out of 2 in 2.0s /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/execroot/ci/bazel-out/local-dbg/testlogs/test/integration/ssl_integration_test/test.log Executed 111 out of 111 tests: 108 tests pass and 3 fail locally. There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are. root@8766e71f0008:/source# ./tools/stack_decode.py /build/tmp/_bazel_bazel/436badd4919a15958fa3800a4e21074a/execroot/ci/bazel-out/local-dbg/bin/test/integration/http2_upstream_integration_test.runfiles/ci/test/integration/http2_upstream_integration_test [2017-05-19 09:38:02.220][17271][critical][assert] assert failure: path != nullptr: test/test_common/environment.cc:48 [2017-05-19 09:38:02.220][17271][critical][backtrace] Caught Aborted, suspect faulting address 0x4377 [2017-05-19 09:38:02.223][17271][critical] Backtrace (most recent call first) from thread 0: #1 ?? ??:0 #2 ?? ??:0 #3 #4 Envoy::TestEnvironment::getCheckedEnvVar(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) at environment.cc:48 (discriminator 1) #5 main at main.cc:23 (discriminator 2) #6 #7 ?? ??:0 #8 #9 _start at ??:? #10

sync/merge from upstream, integration test failures persist.

jwfang · 2017-05-19T10:50:53Z

i did the following to building envoy:

[vagrant@localhost ~]$ sudo docker pull lyft/envoy-build
Using default tag: latest
latest: Pulling from lyft/envoy-build
aafe6b5e13de: Already exists
0a2b43a72660: Already exists
18bdd1e546d2: Already exists
8198342c3e05: Already exists
f56970a44fd4: Already exists
10e7822aad27: Already exists
2f4f7cb8f162: Already exists
96778b1d5664: Already exists
Digest: sha256:b3196cd595fae6cc2e5f887917dbd63f95389d6a484b17f02e8b081c08c45b9a
Status: Image is up to date for lyft/envoy-build:latest

[vagrant@localhost ~]$ sudo docker run -ti --rm -v /home/vagrant/tmp/envoy-build-lyft:/build -v /home/vagrant/src/lyft/envoy:/source lyft/envoy-build bash
root@7d1b733ce347:/# cd /source/
root@7d1b733ce347:/source# ./ci/do_ci.sh bazel.dev

complete test output is in commit message for bcd7ba6

this output from stack_decode.py is really confusing:

[2017-05-19 09:38:02.220][17271][critical][assert] assert failure: path != nullptr: test/test_common/environment.cc:48

run from ./ci/run_envoy_docker.sh
rm -rf ~/tmp/envoy-build && ENVOY_DOCKER_BUILD_DIR=~/tmp/envoy-build sudo ./ci/run_envoy_docker.sh './ci/do_ci.sh bazel.dev'
still see the failure.

do the following changes

[vagrant@localhost envoy]$ git diff test/integration/fake_upstream.cc
diff --git a/test/integration/fake_upstream.cc b/test/integration/fake_upstream.cc
index 1ca2014e..3b685319 100644
--- a/test/integration/fake_upstream.cc
+++ b/test/integration/fake_upstream.cc
@@ -131,8 +131,9 @@ FakeHttpConnection::FakeHttpConnection(QueuedConnectionWrapperPtr connection_wra
   if (type == Type::HTTP1) {
     codec_.reset(new Http::Http1::ServerConnectionImpl(connection_, *this));
   } else {
+    Http::Http2Settings http2_settings;
     codec_.reset(
-        new Http::Http2::ServerConnectionImpl(connection_, *this, store, Http::Http2Settings()));
+        new Http::Http2::ServerConnectionImpl(connection_, *this, store, http2_settings));
     ASSERT(type == Type::HTTP2);
   }

failures persist.

i also run from root without sudo, got the same errors.

BTW, i didn't see this error from latest (56a8c03) upstream master.

since my CI checks also failed, so i guess maybe it's not related to my environments/setups.

is this https://github.com/jwfang/envoy/blob/431c909dcd7d70686cc9986c9ef019bef8942ae2/test/common/http/http2/codec_impl_test.cc#L28 causing the wired behaviour ? will try when at work.

htuch · 2017-05-19T14:23:31Z

+  uint32_t max_concurrent_streams_;
+  uint32_t initial_window_size_;
+
+  static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024;


Nit: just a minor style note while looking at this diff, it would be good to make the capitalization more consistent for the static consts in this header (below is all caps, above is camel case).

these are copied from the old codes. :)
should i go all caps or camel case ?

We don't really have consistent style here unfortunately. I would probably s/DisableDynamicHPACKTable/DISABLE_DYNAMIC_HPACK_TABLE

mattklein123

In general on the right track. Obviously needs all the JSON parsing support, doc updates, etc.

mattklein123 · 2017-05-19T17:33:20Z

+  uint32_t max_concurrent_streams_;
+  uint32_t initial_window_size_;
+
+  static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024;


We don't really have consistent style here unfortunately. I would probably s/DisableDynamicHPACKTable/DISABLE_DYNAMIC_HPACK_TABLE

mattklein123 · 2017-05-19T17:33:50Z

+    static const uint64_t DisableDynamicHPACKTable = 0x1;
+  };
+
+  Http2Settings()


nit: drop constructor definition and just do inline init below. E.g.,
uint64_t codec_options_{0};

etc

didn't know that could be done, from pre-2010 c++ age. thanks

mattklein123 · 2017-05-19T17:34:18Z


 /**
- * A list of options that can be specified when creating a codec.
+ * http/2 settings


nit: "http/2 codec settings"

mattklein123 · 2017-05-19T17:34:47Z

  /**
-   * @return uint64_t HTTP codec options for HTTP connections created on behalf of this cluster.
-   *         @see Http::CodecOptions.
+   * @return Http::Http2Settings for http/2 connections created on behalf of this cluster.


nit: const Http::Http2Settings&

sorry, it's too obvious

mattklein123 · 2017-05-19T17:35:36Z

-                                    DEFAULT_WINDOW_SIZE - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE);
+                                    http2_settings.initial_window_size_ -
+                                        NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE);
+  // TODO: ensure mimium initial_window_size from json schema


Is this still a TODO?

Might also be a good place for an ASSERT() checking that http2_settings.initial_window_size_ is greater than NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE.

missed it, and exactly what i am going to do.

mattklein123 · 2017-05-19T17:37:55Z

  MOCK_CONST_METHOD0(perConnectionBufferLimitBytes, uint32_t());
  MOCK_CONST_METHOD0(features, uint64_t());
-  MOCK_CONST_METHOD0(httpCodecOptions, uint64_t());
+  MOCK_CONST_METHOD0(http2Settings, const Http::Http2Settings&());


The tests are most likely failing because you need to setup a default ON_CALL return a default Http2Settings object. You can verify by running the tests locally with --test_arg="--gmock_verbose=info"

by tests failing, do you mean unit test or integration tests?
it seems all the orignal unit tests run fine, but integration tests keep failing. i guess you already saw my earier comment on this.

not familiar with gtest/gmock, will try as you suggested and invest it.

thank you @mattklein123 , i fixed the integration tests failures by add ON_CALL.

sync/merge upstream.

…pSettings()

jwfang · 2017-05-22T10:48:05Z

can some explained these to me ? it seems both wired and interesting.
https://github.com/jwfang/envoy/blob/issue988/test/common/http/utility_test.cc#L19-L21

i saw Envoy::Router::RetryPolicy and its tests don't need declarations like this.

jwfang · 2017-05-22T12:04:25Z


 // Satisfy linker
-const uint64_t CodecOptions::NoCompression;
+const uint64_t Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE;


need these, else link error with undefined symbols.

but why ？

Without this there is no storage actually assigned to store this static variable. That may be OK: if you never require actual storage for the constant int the compiler doesn't complain. But if the static const is passed by reference or it's address taken at some point in the code then it needs an actual storage location (if it's passed by reference someone might take the address later). This means it must be defined (it's declared in the header) in a single CPP file.

Why does it matter here and not in the Envoy executable? In the Envoy executable case they're never passed by reference so the compiler lets you get away with not defining the variable.

got it, thank you for detailed explanation.

i guess it's here that uses the const static by reference:
https://github.com/google/googletest/blob/59c795ce08be0c8b225bc894f8da6c7954ea5c14/googletest/include/gtest/gtest.h#L1419-L1420

Yup, that looks like it. Good find.

dnoe

A few comments to start. Is the config actually validated against the schema minimum and maximum?

It would be good to see some more tests that exercise the feature and corner cases (try some invalid values, etc)

dnoe · 2017-05-22T19:22:17Z

+  initial_window_size
+    *(optional, integer)* `Initial flow-control window`_ size. Valid values range from 65536
+    (default window size plus 1) to 2147483647 (2^31 - 1), so we only support increase default
+    initial window size now. Default is 268435456 (256 * 1024 * 1024).


"so only increases to the default initial window size are supported" would be a bit clearer.

dnoe · 2017-05-22T19:31:03Z


-  if (codec_options & CodecOptions::NoCompression) {
+  if (http2_settings.codec_options_ &
+      Http::Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE) {


Could this be moved into a boolean method in CodecOptions? It would make the code clearer to read here.

yes, since we are using a struct now, we can add additional fields easily.
so the flags as bool no needed any more.

dnoe · 2017-05-22T19:32:01Z

-                                    DEFAULT_WINDOW_SIZE - NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE);
+                                    http2_settings.initial_window_size_ -
+                                        NGHTTP2_INITIAL_CONNECTION_WINDOW_SIZE);
+  // TODO: ensure mimium initial_window_size from json schema


Is this still a TODO?

dnoe · 2017-05-22T19:33:25Z

+  static const uint32_t DEFAULT_MAX_CONCURRENT_STREAMS = 1024;
+  // For now just set all window sizes (stream and connection) to 256MiB. We can adjust later if
+  // needed.
+  static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = 256 * 1024 * 1024;


Note for myself: this is only moved to a different file, the value is unchanged.

dnoe · 2017-05-22T19:45:40Z

-   * @return uint64_t parse a "http_codec_options" JSON field and turn it into a bitmask of
-   *         CodecOption values.
+   * @return Http2Settings parse "http_codec_options" and "http2_settings" JSON fields and return
+   *         a Http2Settings.


I know it matches the phrasing from before but this PR is a good opportunity to improve this return documentation which reads awkwardly. It should say what it returns such as: "@return Http2Settings An Http2Settings populated from the http_codec_options and http2_settings JSON fields"

dnoe · 2017-05-22T20:11:21Z


 // Satisfy linker
-const uint64_t CodecOptions::NoCompression;
+const uint64_t Http2Settings::CodecOptions::DISABLE_DYNAMIC_HPACK_TABLE;


Without this there is no storage actually assigned to store this static variable. That may be OK: if you never require actual storage for the constant int the compiler doesn't complain. But if the static const is passed by reference or it's address taken at some point in the code then it needs an actual storage location (if it's passed by reference someone might take the address later). This means it must be defined (it's declared in the header) in a single CPP file.

Why does it matter here and not in the Envoy executable? In the Envoy executable case they're never passed by reference so the compiler lets you get away with not defining the variable.

dnoe · 2017-05-22T20:16:29Z

-class CodecOptions {
-public:
-  static const uint64_t NoCompression = 0x1;
+struct Http2Settings {


My personal preference would be to make this a class that hides a bit of it's own logic - I touched on this a bit in another comment. It would enable writing code like calling "http2settings.use_compression()" to find out if compression is enabled rather than requiring the caller to understand that codec_options_ is really a bit field.

i remove the CodecOptions entirely, and add a new hpack_table_size config.

if this is acceptable, then i will finish the docs.

If it's only the equivalent of the getter logic now I think it's fine (but @dnoe makes a valid point in general).

dnoe · 2017-05-22T20:24:22Z

        Json::Factory::loadFromString("{\"http_codec_options\": \"no_compression\"}");
-    EXPECT_EQ(CodecOptions::NoCompression, Utility::parseCodecOptions(*json));
+    EXPECT_EQ(Http2Settings::CodecOptions::DisableDynamicHPACKTable,
+              Utility::parseHttp2Settings(*json).codec_options_);


I think there's more to check here. The other values should be set to their defaults?

dnoe · 2017-05-22T20:25:47Z

  {
    Json::ObjectSharedPtr json = Json::Factory::loadFromString("{\"http_codec_options\": \"foo\"}");
-    EXPECT_THROW(Utility::parseCodecOptions(*json), EnvoyException);
+    EXPECT_THROW(Utility::parseHttp2Settings(*json), EnvoyException);


You can use EXPECT_THROW_WITH_MESSAGE here, this will make sure it's not hitting some other code that throws EnvoyException.

dnoe · 2017-05-22T20:28:29Z

+                                           Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE),
+                    Http2SettingsTestParam(Http2Settings::CodecOptions::DisableDynamicHPACKTable,
+                                           Http2Settings::DEFAULT_MAX_CONCURRENT_STREAMS,
+                                           Http2Settings::DEFAULT_INITIAL_WINDOW_SIZE)));


You could expand the testing by also parameterizing this test case with the edge cases of valid values for max_concurrent_streams and default_initial_window_size.

1. remove Http2Settings::CodecOptions entirely; 2. change DISABLE_DYNAMIC_HPACK_TABLE to Http2Settings.hpack_table_size_; 3. keep http_codec_options.no_compression config for compatibility, but allow overrides with http2_settings.hpack_table_size.

htuch

Thanks @jwfang, appreciate the effort you've made in putting together a solid PR here. Let's get this shipped, you've done a great job in this review. I think we should revert back the minimum max concurrent streams to 1, I don't think there is any reasonable use of a zero value here, given the interest of testing simplicity and getting this shipped. @PiotrSikora any objections?

htuch · 2017-05-26T02:25:11Z

+    the dynamic HPACK table. Valid values range from 0 to 16777216 (2^24) and defaults to 4096,
+    with 0 effectively disables header compression.
+
+    NOTE: A 16MiB (2^24) maximum table size seems to be more than enough.


I think this belongs in implementation notes, not in the user facing docs.

this seems wired, the following is not there:

NOTE: A 16MiB (2^24) maximum table size seems to be more than enough.

i guess same problem with comment test for below:

if (http2_settings->hasObject("hpack_table_size") && ret.hpack_table_size_ != 0) {

htuch · 2017-05-26T02:25:19Z

+    *(optional, integer)* `Maximum concurrent streams`_ allowed on one HTTP/2 connection. Valid values
+    range from 1 to 536870912 (2^29) and defaults to 1024.
+
+    NOTE: Total HTTP/2 streams is 2^31, one side (client/server) is 2^30. To be safe, we use 2^29.


all NOTE except we doesn't support decrease window are removed

htuch · 2017-05-26T02:32:31Z

  for (const std::string& option : StringUtil::split(options, ',')) {
    if (option == "no_compression") {
-      ret |= CodecOptions::NoCompression;
+      if (http2_settings->hasObject("hpack_table_size") && ret.hpack_table_size_ != 0) {


Can you add a test for this?

it seems you're seeing an outdated version, :)

it's tested by this, if i am not misunderstanding you.

+ auto json = Json::Factory::loadFromString(R"raw({ + "http_codec_options": "no_compression", + "http2_settings" : { + "hpack_table_size": 1 + } + })raw"); + EXPECT_THROW_WITH_MESSAGE( + Utility::parseHttp2Settings(*json), EnvoyException, + "'http_codec_options.no_compression' conflicts with 'http2_settings.hpack_table_size'"); } }

htuch · 2017-05-29T22:44:51Z

+
+.. _config_http_conn_man_http2_settings_initial_window_size:
+
+  initial_window_size


Nit: can you rename this initial_stream_window_size now that we also have initial_connection_window_size?

htuch · 2017-05-29T22:45:26Z

+  // TODO(jwfang): support other HTTP/2 settings
+  uint32_t hpack_table_size_{DEFAULT_HPACK_TABLE_SIZE};
+  uint32_t max_concurrent_streams_{DEFAULT_MAX_CONCURRENT_STREAMS};
+  uint32_t initial_window_size_{DEFAULT_INITIAL_WINDOW_SIZE};


Nit: DEFAULT_INITIAL_STREAM_WINDOW_SIZE.

htuch · 2017-05-29T22:51:09Z


-class Http2CodecImplTest : public testing::TestWithParam<uint64_t> {
+typedef ::testing::tuple<uint32_t, uint32_t, uint32_t, uint32_t> http2SettingsTuple;
+typedef ::testing::tuple<http2SettingsTuple, http2SettingsTuple> http2SettingsTupleTuple;


Nit: more descriptive type name than ...TupleTuple please. Also, Please start type names with capital letters, e.g. Http2SettingsTuple.

done.
i left the client/server_http2settings_ member variables there since i thought they maybe useful
if we need to expand/adapt the test later.

htuch · 2017-05-29T22:52:06Z

+typedef ::testing::tuple<uint32_t, uint32_t, uint32_t, uint32_t> http2SettingsTuple;
+typedef ::testing::tuple<http2SettingsTuple, http2SettingsTuple> http2SettingsTupleTuple;
+
+static Http2Settings http2SettingsFromTuple(const http2SettingsTuple& tp) {


Nit: slight preference to use an anonymous namespace rather than static in C++.

another nice trick. thanks.

htuch · 2017-05-29T22:56:45Z

+}
+
+// skip test if server-side max concurrent streams is 0
+#define SKIP_TEST_IF_NOT_SUITABLE                                                                  \


If you're going to make this zero (and I think it's fine to leave it as 1), then you probably do want to use gmock to verify that we get the right behavior here.

PiotrSikora · 2017-05-30T01:51:48Z

@jwfang @htuch Agreed, if MAX_CONCURRENT_STREAMS 0 makes it harder to test and requires workarounds, then I'm fine with leaving minimum value as 1 for now.

PiotrSikora · 2017-05-30T03:03:24Z

+  // TODO(jwfang): make this 0 to support decrease window size
+  static const uint32_t MIN_INITIAL_WINDOW_SIZE = (1 << 16) - 1;
+  // initial stream-level value from HTTP/2 spec, same as NGHTTP2_INITIAL_WINDOW_SIZE from nghttp2
+  static const uint32_t DEFAULT_INITIAL_WINDOW_SIZE = (1 << 16) - 1;


I've noticed that you've changed the default from current 256MB to 64KB resulting in 4096x smaller window size.

As previously noted, 256MB effectively disables flow control, so I definitely agree that we should lower it, but such drastic change will result in different traffic characteristics, and therefor should be committed separately, otherwise we're risking reverting this commit as a whole.

Maybe let's leave it as 256MB in this PR and let's change the default to 1MB in subsequent PR?

@htuch any thoughts?

jwfang · 2017-05-30T05:25:54Z

thanks @htuch @PiotrSikora .

i summarized desired min/max/default values changes below for the this PR:

hpack_table
none
max_concurrent_streams
change minimum to 1
initial_stream_window
change default to 256MB
initial_connection_window
also default to 256MB

new PR:
change both default window size to 64KB

potential TODOs, will results in TODO comments in codes:

hpack_table
none
max_concurrent_streams
minimum to 0
initial_stream_window
minimum to 0
initial_connection_window
minimum to 0
3,4 may not make any sense unless we can increase the window back later some how, but that's another story.

htuch · 2017-05-30T14:02:31Z

@jwfang SGTM. @PiotrSikora I think we'll be effectively lowering the buffer size and flow control window when implementing the buffer management proposal in #150 (which continues to exist as a P2 for me), so I'm not sure if we need to go ahead and lower it before then. I agree that if we do lower it that 64KB might be too low and have potential downside performance effects in a high bandwidth datacenter or cloud environment.

htuch · 2017-05-30T14:04:04Z

 .. _config_http_conn_man_http_codec_options:

 http_codec_options
+  **DEPRECATED**, use :ref:`http2_settings <config_http_conn_man_http2_settings>` instead.


Can you also update https://github.com/lyft/envoy/blob/master/DEPRECATED.md? Thanks.

jwfang · 2017-05-31T06:35:41Z

@htuch @PiotrSikora i change minimum max_concurrent_stream to 1 and default stream/connection_window_size to 256MB.

also some other changes from the review. hope nothing is left.

BTW: weird the new commits show under my last comment, may need to comment before push. :)

htuch · 2017-05-31T16:04:48Z

@jwfang Thanks. Maybe something with timestamps was messed up (ignore my previous comment about force push, I see the diffs above).

htuch · 2017-05-31T16:06:48Z

@ccaraman @RomanDzhabarov @junr03 Do one of you want to take a look at this?

junr03

LGTM, just a few doc nits.

junr03 · 2017-05-31T17:26:35Z

+
+  hpack_table_size
+    *(optional, integer)* `Maximum Table Size`_ (in octets) that the encoder is permitted to use for
+    the dynamic HPACK table. Valid values range from 0 to 4294967295 (2^32 - 1) and defaults to 4096,


defaults to 4096. 0 effectively disables header compression.

junr03 · 2017-05-31T17:27:23Z

+    (2^16 - 1, HTTP/2 default) to 2147483647 (2^31 - 1, HTTP/2 maximum) and defaults to 268435456
+    (256 * 1024 * 1024).
+
+    NOTE: 65535 is the initial window size from HTTP/2 spec. We only support increase the default window


s/increase/increasing

junr03 · 2017-05-31T17:36:55Z

+  Currently supported settings are:
+
+  hpack_table_size
+    *(optional, integer)* `Maximum Table Size`_ (in octets) that the encoder is permitted to use for


for consistency, can you change the links here to `link-name <link>`_. And delete the links at the end of this section.

jwfang · 2017-06-01T02:40:25Z

@junr03 thanks for your review, docs updated.
@htuch the commits showing under previous comment things indeed caused by timestamp, my VM's
date lags by days during the weekend.

junr03

lgtm

htuch · 2017-06-01T12:01:11Z

@jwfang Merged, thanks. Can we also close out #987?

jwfang · 2017-06-01T14:32:33Z

thanks @htuch .

#987 is different from this one, the title is misleading.

this PR is to advertise our http/2 setting to our peer, #987 is for envoy to respect peer's setting.
but according to @mattklein123 :

If we overflow the other side, the code will correctly handle the REFUSED_STREAM reset that happens.

i am closing it and will create a new one if really needed.

mattklein123 · 2017-06-06T18:05:17Z

+         http2_settings.initial_connection_window_size_ <=
+             Http2Settings::MAX_INITIAL_CONNECTION_WINDOW_SIZE);
+
+  std::vector<nghttp2_settings_entry> iv;


For posterity, for perf we should switch this to a fixed size array with a pre-defined max size. Added to #210.

Automatic merge from submit-queue. [DO NOT MERGE] Auto PR to update dependencies of proxy This PR will be merged automatically once checks are successful. ```release-note none ```

Description: Changes filter instantiation to be a per-stream basis at the Android platform layers. Risk Level: Low Testing: Local and CI Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

Http::CodecOptions struct and interface changes, make it compiles

77459a1

jwfang mentioned this pull request May 18, 2017

http/2 SETTINGS_MAX_CONCURRENT_STREAMS : downstream/listener #988

Closed

mattklein123 reviewed May 18, 2017

View reviewed changes

方俊武 added 4 commits May 19, 2017 10:58

following mattklein123's review

b75039a

refactor around original CodecOptions

6d1a199

1. change original uint32_t CodecOptions to Http2Settings, and move CodecOptions into Http2Settings; 2. default ctor for Http2Settings, and move default consts into Http2Settings; 3. hard code maxConcurrentStreams return temporarily till PR #981.

Merge remote-tracking branch 'upstream/master' into issue988

794614b

sync/merge from upstream, integration test failures persist.

jwfang changed the title ~~downstream/listener side SETTINGS_MAX_CONCURRENT_STREAMS~~ downstream/listener side max concurrent streams setting, WIP May 19, 2017

jwfang changed the title ~~downstream/listener side max concurrent streams setting, WIP~~ downstream/listener side max concurrent streams setting (#988), WIP May 19, 2017

correct comment; and try trigger another CI

431c909

htuch reviewed May 19, 2017

View reviewed changes

mattklein123 reviewed May 19, 2017

View reviewed changes

方俊武 added 6 commits May 22, 2017 11:43

adapted for latest code review, except ON_CALL

94fad70

Merge remote-tracking branch 'upstream/master' into issue988

f4dcff7

sync/merge upstream.

fix integration test failures by add ON_CALL for MockClusterInfo::htt…

d4251d4

…pSettings()

parse http2_settings json config

b7ee834

fix schema type spell error

164fe93

docs for http2_settings json config

4e7251b

doc defaults for max_concurrent_streams/initial_window_size

d02e90d

jwfang commented May 22, 2017

View reviewed changes

docs updates

1fe2009

RomanDzhabarov assigned dnoe May 22, 2017

dnoe suggested changes May 22, 2017

View reviewed changes

方俊武 added 4 commits May 23, 2017 10:40

only update initial window size when needed

7709cd8

docs update from review

b072dfb

expand Http2Settings tests for min/max of all fields

f9d117a

jwfang added 5 commits May 29, 2017 00:32

format fix

685e811

add notes in DEPRECATED.md

6476bad

fix max_concurrent_streams json schema

2a5116c

minor tuning

8b635f4

add missing assert

73dcf19

htuch reviewed May 29, 2017

View reviewed changes

PiotrSikora reviewed May 30, 2017

View reviewed changes

htuch reviewed May 30, 2017

View reviewed changes

htuch previously approved these changes May 31, 2017

View reviewed changes

junr03 reviewed May 31, 2017

View reviewed changes

update docs link

6c5be11

jwfang dismissed htuch’s stale review via 6c5be11 June 1, 2017 02:29

fix docs caps

a38d334

junr03 approved these changes Jun 1, 2017

View reviewed changes

htuch merged commit ea46f14 into envoyproxy:master Jun 1, 2017

mattklein123 reviewed Jun 6, 2017

View reviewed changes


		.. _config_http_conn_man_http2_settings_initial_window_size:

		initial_window_size

Conversation

jwfang commented May 18, 2017

Uh oh!

jwfang commented May 18, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwfang commented May 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwfang May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwfang commented May 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwfang May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jwfang commented May 19, 2017 •

edited

Loading

jwfang May 23, 2017 •

edited

Loading

jwfang May 23, 2017 •

edited

Loading

jwfang May 23, 2017 •

edited

Loading

jwfang May 31, 2017 •

edited

Loading