Tcp flow control by alyssawilk · Pull Request #1217 · envoyproxy/envoy

alyssawilk · 2017-07-06T19:07:28Z

Early steps for #150, starting with something "simple" (heh)

Adding watermark functions to the Connection and ConnectionCallbacks
Having both the base network ConnectionImpl and the SSL version call Watermark callbacks
Having the tcp filter react to Watermark callbacks by enabling/disabling reads

This includes tests of *::ConnectionImpl and new TCP/TLS tests which exert the watermark code thanks to our read limits being willfully ignored (we read 16384 rather than min(16834, read_buffer_limit_))

htuch

Looks good, nice progress!

htuch · 2017-07-06T21:31:58Z

source/common/network/connection_impl.cc

+}
+
+void ConnectionImpl::checkForHighWatermark() {
+  if (above_high_watermark_called_ || high_watermark_ == 0 ||


There's some asymmetry here that is probably fine, but might be worth calling out somewhere. You'll never receive an onBelowWBLowWatermark() unless onAboveWBHighWatermark() has been called, but the other direction doesn't hold.

Yeah, per my other comment, I would add as many ASSERTs as make sense to code like this.

I've called it out more explicitly in include/envoy/network/connection.h let me know if you think it should go elsewhere as well!

htuch · 2017-07-06T21:38:02Z

source/common/network/connection_impl.h

  Buffer::Instance& getWriteBuffer() override { return *current_write_buffer_; }

+  void replaceWriteBufferForTest(std::unique_ptr<Buffer::OwnedImpl> new_buffer) {
+    write_buffer_ = std::move(new_buffer);


It feels a bit scary reaching in and mutating internal state from a test (as opposed to helper methods to just inspect state). I wonder if there is a way to factory-ify this to allow mocking via dependency injection...

Agreed. The general pattern we have been doing is to make something like BufferFactory which returns a buffer. Then in a test you can hold on to the pointer to do things. Search for "Factory" and you will see many examples.

SGTM. This turns out to involve touching a handful of new files (and the test refactors were already pretty large) so if no one objects I'm going to the test changes and the buffer factory out into their own separate patch since this one is already pretty large.

That's fine with me if that is easier.

Done, over in #1234. Feel free to ignore this one until that is merge in, or I can attempt to rebase if you'd prefer to review things in parallel.

htuch · 2017-07-06T21:38:28Z

source/common/network/connection_impl.h

  uint64_t last_write_buffer_size_{};
  std::unique_ptr<BufferStats> buffer_stats_;
+  // Tracks the number of times reads have been disabled.  If N different components call
+  // readDisabled(true) this allows the connection to only resume reasd when readDisabled(false)


Nit: s/reasd/reads/

htuch · 2017-07-06T21:52:21Z

source/common/network/listener_impl.cc

+  if (options_.per_connection_buffer_limit_bytes_ > 0) {
+    new_connection->setWriteBufferWatermarks(options_.per_connection_buffer_limit_bytes_ / 2,
+                                             options_.per_connection_buffer_limit_bytes_ + 1);
+  }


Can we refactor this logic to be in one place?

Oops - missed this one. I'd be inclined to merge setReadBufferLimit and setWriteBufferWatermarks into setConnectionBufferLimits which just does both under the hood. I'll see if that works cleanly in code and docs tomorrow.

htuch · 2017-07-06T21:54:31Z

test/common/network/connection_impl_test.cc

 TEST_P(ConnectionImplDeathTest, BadFd) {
-  Event::DispatcherImpl dispatcher;
-  EXPECT_DEATH(ConnectionImpl(dispatcher, -1,
+  Event::DispatcherImpl dispatcher_;


_ suffix for local variables?

mattklein123

Overall looks great. A few quick pile on comments.

mattklein123 · 2017-07-06T23:37:00Z

include/envoy/network/connection.h

+   * @param high_watermark if the connection has more bytes than this buffered,
+   * onAboveWriteBufferHighWatermark will be called.
+   */
+  virtual void setWriteBufferWatermarks(size_t low_watermark, size_t high_watermark) PURE;


nit: prefer fixed sizes in Envoy code (probably uint32_t here but uint64_t is fine if you want).

mattklein123 · 2017-07-06T23:39:52Z

source/common/filter/tcp_proxy.cc

+  read_callbacks_->connection().readDisable(disable);
+}
+
+void TcpProxy::DownstreamCallbacks::onAboveWriteBufferHighWatermark() {


Is it worth it here to add ASSERTS in all of these functions that basically assert that we aren't getting multiple watermark callbacks? Or can that happen? I'm thinking something along the lines of ASSERT(!parent_.upstreamReadDisabled()). (Similar in other places).

connections can have the read disabled by other means so if we check the actual state of the connection we can only verify that we enable actually disabled sockets (not that we disable enabled sockets) We could track local state by adding 2 booleans to the tcp proxy filter which are only used for debug, which I'd be happy to do - flow control is tricky enough to get right I'm happy to have extra asserts too!

I would be in favor of adding the booleans (at least for now, with a TODO to clean them up once we have production experience) since this code is super tricky to get right and more checking is better IMO. I will leave it up to you to decide either way.

Works for me. Flow control changes are inherently dangerous as you point out.
Added asserts and the the tests pass 5k runs without flakes once I got the waitForDisconnect fix merged in.

mattklein123 · 2017-07-06T23:44:30Z

source/common/network/connection_impl.cc

 }

+void ConnectionImpl::setWriteBufferWatermarks(size_t new_low_watermark, size_t new_high_watermark) {
+  ENVOY_CONN_LOG(trace, "Setting watermarks: {} {}", *this, new_low_watermark, new_high_watermark);


nit: I would probably do this one at debug level. (We should codify this but in general I've been doing trace level for anything that is in the data path and would happen during the connection, like writing bytes, reading bytes, etc., but this is more of a one time thing).

mattklein123 · 2017-07-06T23:45:24Z

source/common/network/connection_impl.cc

+}
+
+void ConnectionImpl::checkForHighWatermark() {
+  if (above_high_watermark_called_ || high_watermark_ == 0 ||


Yeah, per my other comment, I would add as many ASSERTs as make sense to code like this.

mattklein123 · 2017-07-06T23:46:22Z

source/common/network/connection_impl.h

  Buffer::Instance& getWriteBuffer() override { return *current_write_buffer_; }

+  void replaceWriteBufferForTest(std::unique_ptr<Buffer::OwnedImpl> new_buffer) {
+    write_buffer_ = std::move(new_buffer);


Agreed. The general pattern we have been doing is to make something like BufferFactory which returns a buffer. Then in a test you can hold on to the pointer to do things. Search for "Factory" and you will see many examples.

mattklein123 · 2017-07-06T23:46:56Z

source/common/network/connection_impl.h

  Address::InstanceConstSharedPtr local_address_;
  Buffer::OwnedImpl read_buffer_;
-  Buffer::OwnedImpl write_buffer_;
+  std::unique_ptr<Buffer::OwnedImpl> write_buffer_{new Buffer::OwnedImpl};


Nit: Buffer::InstancePtr

mattklein123 · 2017-07-06T23:47:14Z

source/common/network/connection_impl.h

+  // buffer passes |high_watermark_|, onAboveWriteBufferHighWatermark will be called to disable
+  // reading further data.  When the buffer drains below |low_watermark_|,
+  // onBelowWriteBufferLowWatermark will be called to resume reads.
+  size_t high_watermark_{0};


same comment about uint32_t or uint64_t

mattklein123 · 2017-07-06T23:48:22Z

source/common/upstream/upstream_impl.cc

-  connection->setReadBufferLimit(cluster.perConnectionBufferLimitBytes());
+  uint32_t buffer_limit = cluster.perConnectionBufferLimitBytes();
+  connection->setReadBufferLimit(buffer_limit);
+  connection->setWriteBufferWatermarks(buffer_limit / 2, buffer_limit + 1);


Same here per @htuch other comment. I would recommend adding some type of watermark utility class that can be used to setup nice defaults on a connection.

rshriram · 2017-07-10T16:51:50Z

May be I missed this somewhere, but is there an option to configure the watermark (or the buffer size for upstream) for plain TCP connections?x

alyssawilk · 2017-07-10T17:01:07Z

Yep, this one is pure TCP - perhaps attaching it to the H2 bug was somewhat confusing, sorry. It's configured via the existing setting in the TCP filter and upstream config "per_connection_buffer_limit_bytes" since that seemed general enough to continue using. I'll do filters and H2 level flow control in a (series of) follow up You can see the example config in additions to test/config/integration/tcp_proxy.json to see a second TCP listener with limits and a new cluster with upstream limits.

…

On Mon, Jul 10, 2017 at 12:51 PM, Shriram Rajagopalan < ***@***.***> wrote: May be I missed this somewhere, but is there an option to configure the watermark (or the buffer size for upstream) for plain TCP connections?x — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1217 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARYFveoSNFDF3-Ygq72VwQKLjzuCB1w0ks5sMlasgaJpZM4OQDoP> .

mattklein123

looks awesome modulo a few small things and the remaining comment of centralizing the / 2 setup logic somewhere.

mattklein123 · 2017-07-10T23:45:42Z

include/envoy/network/connection.h

+   * and onBelowWriteBufferHighWatermark callbacks.
+   * The connection is assumed to start out with less than high_watermark
+   * worth of data buffered, so onAboveWriteBufferHighWatermark will always be
+   * called before onAboveWriteBufferHighWatermark


s/onAboveWriteBufferHighWatermark/onBelowWriteBufferLowWatermark ?

mattklein123 · 2017-07-10T23:46:37Z

include/envoy/network/connection.h

+
+  /**
+   * Sets the high and low watermarks which trigger onAboveWriteBufferHighWatermark
+   * and onBelowWriteBufferHighWatermark callbacks.


s/onBelowWriteBufferHighWatermark/onBelowWriteBufferLowWatermark ?

mattklein123 · 2017-07-10T23:47:20Z

include/envoy/network/connection.h

+   * worth of data buffered, so onAboveWriteBufferHighWatermark will always be
+   * called before onAboveWriteBufferHighWatermark
+   * @param low_watermark if the connection was above the high watermark and the
+   * connection buffer is drained below this many bytes, onBelowWriteBufferHighWatermark will be


s/onBelowWriteBufferHighWatermark/onBelowWriteBufferLowWatermark ?

mattklein123 · 2017-07-10T23:51:13Z

test/common/ssl/connection_impl_test.cc

-
-    Network::ListenerPtr listener = dispatcher.createSslListener(
-        connection_handler, *server_ctx, socket, listener_callbacks, stats_store,
+  void Initialize(uint32_t read_buffer_limit) {


nit: initialize (I think part of this change is in the other change also, so might be best to get one of these in first and then rebase/focus on the other one).

mattklein123 · 2017-07-10T23:52:16Z

test/mocks/buffer/mocks.h


 namespace Envoy {
+
+class MockBuffer : public Buffer::OwnedImpl {


same comments about method name casing in this PR (also same comment about picking one of the PRs to get in first and then we can do the other one).

mattklein123 · 2017-07-11T01:29:14Z

@alyssawilk before I forget, I think it would be worthwhile to add a stat for tcp_proxy high watermark pause and low watermark resume. Probably:
https://github.com/lyft/envoy/blob/master/source/common/filter/tcp_proxy.h#L27

and

https://github.com/lyft/envoy/blob/master/include/envoy/upstream/upstream.h#L185

If possible I would like to do this in this PR vs. follow up so that when we deploy we can watch the stats.

htuch

LGTM modulo remaining comments from @mattklein123.

htuch · 2017-07-11T02:37:05Z

test/common/network/connection_impl_test.cc

+  Network::MockConnectionCallbacks server_callbacks_;
+  std::shared_ptr<MockReadFilter> read_filter_;
+  std::unique_ptr<MockBuffer> buffer_ptr_{new MockBuffer()};
+  MockBuffer& buffer_{*buffer_ptr_};


You can make this an actual pointer as per earlier thread and idiom discussion if you want.

htuch · 2017-07-11T02:39:48Z

test/mocks/buffer/mocks.h

+    return -1;
+  }
+
+  int bytes_written() const { return bytes_written_; }


Also I'd reiterate the general preference for fixed sized types for quantities here, e.g. uint32_t.

htuch · 2017-07-11T22:18:43Z

source/common/network/connection_impl.cc

+  // enabling/disabling reads from the socket, or allowing more data, but then not triggering
+  // based on watermarks until 2x the data is buffered in the common case.  Given these are all soft
+  // limits we err on the side of buffering more and having better performace.
+  // If the connection class is changed to write-and-flush the high watermark should be changed to


Can you elaborate on what "write-and-flush" means? Is this a flush after every write? Why does this affect high watermark?

…of test rewrites)

alyssawilk · 2017-07-12T18:11:58Z

Added - I am happy to add any and all stats and asserts to make this one minimally painful :-) Is there a non-trivial way to check stats in integration tests? Now that I have stats I'd like to regression test that the tcp proxy test with large writes actually pauses and resumes due to watermarks and I don't see any code to crib off of.

…

On Mon, Jul 10, 2017 at 9:29 PM, Matt Klein ***@***.***> wrote: @alyssawilk <https://github.com/alyssawilk> before I forget, I think it would be worthwhile to add a stat for tcp_proxy high watermark pause and low watermark resume. Probably: https://github.com/lyft/envoy/blob/master/source/common/ filter/tcp_proxy.h#L27 and https://github.com/lyft/envoy/blob/master/include/envoy/ upstream/upstream.h#L185 If possible I would like to do this in this PR vs. follow up so that when we deploy we can watch the stats. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1217 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ARYFvQDX6lNA1v1MaksIDt5axuDb9BL9ks5sMs_vgaJpZM4OQDoP> .

mattklein123 · 2017-07-12T18:28:59Z

@alyssawilk it's not perfect but roughly this will work: https://github.com/lyft/envoy/blob/master/test/integration/ssl_integration_test.cc#L83

mattklein123 · 2017-07-12T18:33:46Z

source/common/filter/tcp_proxy.h

  COUNTER(downstream_cx_total)                                                                     \
-  COUNTER(downstream_cx_no_route)
+  COUNTER(downstream_cx_no_route)                                                                  \
+  COUNTER(upstream_pause_reading)                                                                  \


It would be better IMO if these were defined as upstream cluster stats (see the link I originally referenced). They can then also be used in the HTTP flow and for other proxy scenarios.

P.S. please also document the new stats in the relevant parts of the docs.

Is it reasonable to put upstream/downstream in the filter stats and upstream-pauses (and resumes) in the upstream cluster stats? Adding downstream-pauses in the upstream stats seems odd to me but maybe that's me not having fully absorbed what goes where in envoy yet :-)

Also thanks for the pointer to stats! I'm much happier with (my local, soon upstream) tcp proxy integration tests validating them.

If you take a look at generally the way we do stats, where possible we use cluster for the "upstream" stats. "Downstream" stats are taken care of by the filter itself. In this case its TCP proxy, but in future PRs you will obviously be doing the same type of thing in the HTTP connection manager. This is why today you see only downstream in the above stats and all the upstream stats are in the cluster stats. If you look at existing tcp proxy filter code you will see lots of uses of cluster stats for this purpose. I would just replicate that.

The main reason to do this is for consistency across protocols where possible.

Edit: ah, I think your comment answered this for me. We can only track what the filter does in the filter so I'll just track flow_control related enable/disable

Actually I have a one more questions on this (sorry for the back and forth).

Added upstream_pause_reading_total to cluster stats in my local branch but then it only gets watermark enables/disables. Any preference for a rename for flow_control_[paused|resumed]_reads vs tracking stats in readDisable on the connection?

Given there's already some readEnabling/Disabling going on on the http side I'm not sure if you'd prefer to track the flow control changes separately or if tracking all enables/disables is more useful.

No problem at all with the questions.

I haven't thought through it completely, but I agree that explicitly tracking flow control pause/resume sounds useful and probably what we want. So renaming sounds good. We can always add connection level read disable/enable stats later on if we want. I guess that would be my vote (just rename of what you are doing now). But will defer to your judgement.

Please to not merge yet - I think I had a test flake locally and I'm trying to sort out if it's related to this patch set.

Edit: Ah, annoyingly it's not guaranteed that the large write test backs up - it almost always does on debug builds but very occasionally fails and often fails on tsan.

mattklein123 · 2017-07-13T19:59:07Z

include/envoy/network/connection.h

-   * the write butter.  When enough data is drained from the write buffer,
-   * onBelowWriteBufferHighWatermark is called which results in resuming reads.
+   * onAboveWriteBufferHighWatermark, which allows subscribers to enforce flow control by disabling
+   * reads on the socket funneling data to the write butter.  When enough data is drained from the


s/butter/buffer :)

alyssawilk · 2017-07-14T01:36:33Z

source/common/buffer/watermark_buffer.h

+// buffer is resized (written to or drained), the watermarks are checked.  As the buffer size
+// transitions from under the low watermark to above the high watermark, the above_high_watermark
+// function is called one time. It will not be called again until the buffer is drained below the
+// low watermark, at which point the below_low_watermark function is called.


Matt's earlier comment re:utilities was spot on - once I started on h2 flow control I found myself copy-pasting watermark checks so pulled this out. I believe it will be reusable for at least some of the codec buffers. Either way it guards us from screwing up and missing buffer size changes changes (I missed one in ssl/connection_impl.cc before my own tests caught me) because all edits are done through the wrapper.

@htuch let me know what you think before I go too crazy with the unit tests :-)

mattklein123 · 2017-07-14T18:27:31Z

@alyssawilk I merged the other change so once you merge master on this one I think we should be good to go!

htuch · 2017-07-17T14:04:17Z

source/common/buffer/watermark_buffer.h

+// transitions from under the low watermark to above the high watermark, the above_high_watermark
+// function is called one time. It will not be called again until the buffer is drained below the
+// low watermark, at which point the below_low_watermark function is called.
+class WatermarkBuffer : public Instance {


This is really nice @alyssawilk. The only question I had is whether it makes sense to make this a subclass of OwnedImpl or not? It's probably better as a wrapper, since it has nothing really to do with the OwnedImpl implementation, OTOH you can avoid a few lines of wrapper calls where it would just chain to the parent class. I think what you have is good, if you can add the tests to provide coverage I think we're good to go.

That's basically what I thought, but I'm happy to go either way if @mattklein123 would prefer the perf boost.
Should be up to 100% coverage on the new watermark code now.

Maybe just throw in a TODO about potential perf boost? This change is big enough as it is so if it's working my preference would be to just ship it and iterate later.

Sticking with things as is SGTM.
Having thought more I agree with Harvey that this is more of a code cleanliness choice rather than a perf issue so I'd lean towards adding a TODO iff it shows up in profiles if that's OK

Sure that's fine. I will take a final pass through this change later today.

alyssawilk · 2017-07-18T14:46:34Z

source/common/buffer/watermark_buffer.h

  }
  int write(int fd) override;
+  Event::Libevent::BufferPtr& buffer() override {
+    return static_cast<LibEventInstance&>(*wrapped_buffer_).buffer();


@htuch as discussed this solves the problem where move didn't work in both directions (now tested) because OwnedImpl assumed it was moving another OwnedImpl. The down-side is that WatermarkBuffer now only can do move()s if it wraps an OwnedImpl. That said the Envoy code base already assumes all buffers are OwnedImpls so I don't think we're losing any flexibility, it's just ugly from a purist perspective.

mattklein123

Looks really awesome. Few small comments. Will let @htuch finish out the review and merge.

mattklein123 · 2017-07-18T14:45:37Z

source/common/buffer/watermark_buffer.h

+// transitions from under the low watermark to above the high watermark, the above_high_watermark
+// function is called one time. It will not be called again until the buffer is drained below the
+// low watermark, at which point the below_low_watermark function is called.
+class WatermarkBuffer : public LibEventInstance {


If this is a wrapper around a buffer, does it have to derive from LibEventInstance ? Are the changes to introduce LibEventInstance necessary? (If they are the below static_cast should probably be a dynamic_cast, but it's not clear to me that it's necessary).

Maybe you just answered my comment with your other comment. If so, maybe add more code comments?

mattklein123 · 2017-07-18T14:47:15Z

source/common/event/dispatcher_impl.cc

 #include "envoy/network/listen_socket.h"
 #include "envoy/network/listener.h"

+#include "common/buffer/buffer_impl.h"


Is this needed?

For OwnedImplFactory.

mattklein123 · 2017-07-18T14:49:09Z

source/common/network/connection_impl.cc

-      write_buffer_(dispatcher.getBufferFactory().create()), dispatcher_(dispatcher), fd_(fd),
-      id_(++next_global_id_) {
+      write_buffer_(Buffer::InstancePtr{dispatcher.getBufferFactory().create()},
+                    [&]() -> void { this->onLowWatermark(); },


nit: capture this, not & (everything). Same next line.

mattklein123 · 2017-07-18T14:51:04Z

source/common/network/connection_impl.cc

+  // based on watermarks until 2x the data is buffered in the common case.  Given these are all soft
+  // limits we err on the side of buffering more and having better performace.
+  // If the connection class is changed to write to the buffer and flush to the socket in the same
+  // stack, the high watermark should be changed to the buffer limit without the + 1


Question: Not immediately obvious what the +1 is about. Can you explain a bit more in comment?

mattklein123 · 2017-07-18T14:52:17Z

source/common/upstream/upstream_impl.cc

      cluster.sslContext() ? dispatcher.createSslClientConnection(*cluster.sslContext(), address)
                           : dispatcher.createClientConnection(address);
-  connection->setReadBufferLimit(cluster.perConnectionBufferLimitBytes());
+  uint32_t buffer_limit = cluster.perConnectionBufferLimitBytes();


nit: const uint32_t, or just lose the local variable and do it inline like it was before (might need static_cast).

htuch · 2017-07-18T15:30:20Z

test/common/network/connection_impl_test.cc

+  connect();
+  client_connection_->setBufferLimits(10);
+
+  Runtime::RandomGeneratorImpl rand;


Can we use a seed that can be printed and supplied on the command-line to repeat failed tests? I worry about failures we catch in CI flakes that we can't reproduce otherwise. Maybe the gtest seed (https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#shuffling-the-tests) ?

I don't see an ability to pass seeds to our rng.
I do see a TODO in test/test_common/network_utility.cc for such a feature :-/

This is true, but should be fixed. At a minimum file an issue, add a TODO and reference. We do use RandomGeneratorImpl in a few places in test/ today, but I feel this one in particular is likely to bite us, as the particular read/write schedule that might trigger a bug could be rare.

FWIW, the code is trivial: https://github.com/lyft/envoy/blob/master/source/common/runtime/runtime_impl.h#L31

I would just add a TestRandomGeneratorImpl that doesn't use thread local stuff and allows a seed.

htuch · 2017-07-18T15:31:36Z

source/common/network/connection_impl.cc

+  // based on watermarks until 2x the data is buffered in the common case.  Given these are all soft
+  // limits we err on the side of buffering more and having better performace.
+  // If the connection class is changed to write to the buffer and flush to the socket in the same
+  // stack, the high watermark should be changed from |limit + 1| to |limit|.


I think I'm still not smart enough to understand why this is the case :(

Confusing you and Matt means I'm clearly not explaining it well :-/
Here's one more pass - if this doesn't do it I'd lean towards explaining in person and getting your help clarifying it

mattklein123 · 2017-07-18T16:32:14Z

source/common/network/connection_impl.cc

  // enabling/disabling reads from the socket, or allowing more data, but then not triggering
  // based on watermarks until 2x the data is buffered in the common case.  Given these are all soft
-  // limits we err on the side of buffering more and having better performace.
+  // limits we err on the side of buffering more triggering watermark callbacks less often.


"buffering more and ..."

OK this makes sense to me now, thanks. I guess my follow up question is whether this is needed though. The assumption is that we will be writing limit bytes in the typical case. I kind of doubt that will happen (limits would typically be set with some margin above the typical back and forth sizes). Given that, does the extra complexity and groking time provide enough value? (I don't really feel that strongly about it, mostly asking the question).

Heh, just had the same conversation with Harvey. It is the common case for tcp proxying. I'd updated the comment to this effect but yeah, I'm just as happy removing.

I'm fine keeping it if we think it will provide value in a real scenario (seems like it will).

htuch

Thanks for fixing this, a few comments.

htuch · 2017-07-18T17:55:42Z

test/test_common/utility.h

    EXPECT_EQ(message, std::string(e.what()));                                                     \
  }

+class TestRandomGenerator {


Is there any value in this implementing the RandomGenerator interface?

Not currently. It would require faking out the uuid string which no one needs. No callers actually want the RandomGenerator interface they just want a random number.
I can of course do it anyway if we anticipate the use case.

htuch · 2017-07-18T17:58:54Z

test/test_common/utility.cc

-TestRandomGenerator::TestRandomGenerator() : generator_(SEED) {
-  std::cerr << "TestRandomGenerator running with seed " << SEED;
+TestRandomGenerator::TestRandomGenerator()
+    : generator_(GTEST_FLAG(random_seed) == 0 ? SEED : GTEST_FLAG(random_seed)) {


google/googletest#331 says we should be able to get the current seed with UnitTest::random_seed().

We can but it appears to always be 0 as it was when I tried accessing it via flag.
It appears to only be set if you set --gtest_shuffle which I find to be an uncomfortable hidden dependency I'd rather not depend on.

http://docs.ros.org/api/self_test/html/gtest-all_8cc_source.html -> 5752

htuch · 2017-07-18T17:59:21Z

test/test_common/utility.cc

-  std::cerr << "TestRandomGenerator running with seed " << SEED;
+TestRandomGenerator::TestRandomGenerator()
+    : generator_(GTEST_FLAG(random_seed) == 0 ? SEED : GTEST_FLAG(random_seed)) {
+  const int32_t seed = GTEST_FLAG(random_seed) == 0 ? SEED : GTEST_FLAG(random_seed);


Might be cleaner to make this a const member variable and initialize it once in the member initializor list, then initialize generator_ with seed_.

Sure, it's test code, we can burn the extra memory for a temp varaible.

Automatic merge from submit-queue. Add support for v1 config. **What this PR does / why we need it**: **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes envoyproxy#1217 **Special notes for your reviewer**: **Release note**: ```release-note NONE ```

Description: This PR contains only the header changes from #1209 which were made while developing. Risk Level: Low Testing: N/A, no behavior changes Docs Changes: N/A Release Notes: N/A Signed-off-by: Cerek Hillen <chillen@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

alyssawilk added 7 commits July 6, 2017 12:53

TCP + TLS flow control

cbe959f

reinstating a check

f2ffecb

Adding SSL unit tests

0a9116d

seeing if pointers are ever OK

24b56df

fixing test mocks

2cb1ebf

correcting docs

f07608b

fix_format

36ecdc3

htuch reviewed Jul 6, 2017

View reviewed changes

mattklein123 reviewed Jul 6, 2017

View reviewed changes

alyssawilk added 2 commits July 10, 2017 11:14

Merge branch 'master' into tcp-flow-control

ccea6d4

Addressing some review comments

b846116

alyssawilk mentioned this pull request Jul 10, 2017

Refactoring the connection class to create buffers via factory #1234

Merged

alyssawilk added 3 commits July 10, 2017 16:48

Addressing the rest of the comments not handled by envoyproxy#1234

8fd62ca

Merge branch 'refs/heads/master' into tcp-flow-control

1036f4b

Fixing the new proxy test given envoyproxy#1232

ea9aebb

mattklein123 reviewed Jul 10, 2017

View reviewed changes

htuch reviewed Jul 11, 2017

View reviewed changes

setting all buffer limits with one call

e1a91fa

htuch reviewed Jul 11, 2017

View reviewed changes

alyssawilk added 2 commits July 12, 2017 13:57

Merge branch 'refs/heads/master' into tcp-flow-control (plus a bunch …

9bbc0f4

…of test rewrites)

adding stats

f5ce5f4

mattklein123 reviewed Jul 12, 2017

View reviewed changes

alyssawilk added 3 commits July 12, 2017 16:07

now with proper stats, and tests!

73f80d3

Merge branch 'refs/heads/master' into tcp-flow-control

62098a4

Fixing a test flake - on tsan we don't always get write backups

e15f458

mattklein123 reviewed Jul 13, 2017

View reviewed changes

alyssawilk added 2 commits July 13, 2017 16:03

I can't believe it's not butter!

31b46da

Moving watermark checks to a utility class wrapping the buffer

872e1b3

alyssawilk commented Jul 14, 2017

View reviewed changes

Merge branch 'refs/heads/master' into tcp-flow-control

f373704

htuch reviewed Jul 17, 2017

View reviewed changes

alyssawilk added 2 commits July 17, 2017 10:53

now with better coverage

71792b3

Making move work between watermark buffers and ownedimpl

c6e50dc

alyssawilk commented Jul 18, 2017

View reviewed changes

mattklein123 reviewed Jul 18, 2017

View reviewed changes

Adding more comments for watermark buffers, misc review comments

c925fee

htuch reviewed Jul 18, 2017

View reviewed changes

one more shot!

3bf7617

mattklein123 reviewed Jul 18, 2017

View reviewed changes

alyssawilk added 3 commits July 18, 2017 12:35

more clarity on watermark

b8216e9

Test RNG

d55f9eb

possibly abusing the random seed flag

b79fc5e

htuch reviewed Jul 18, 2017

View reviewed changes

latching rng seed

531b61e

htuch approved these changes Jul 18, 2017

View reviewed changes

mattklein123 approved these changes Jul 18, 2017

View reviewed changes

Merge branch 'refs/heads/master' into tcp-flow-control

69a8e33

mattklein123 merged commit 1580db3 into envoyproxy:master Jul 18, 2017

alyssawilk deleted the tcp-flow-control branch July 20, 2017 14:58


		namespace Envoy {

		class MockBuffer : public Buffer::OwnedImpl {

Conversation

alyssawilk commented Jul 6, 2017

Uh oh!

htuch 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram commented Jul 10, 2017

Uh oh!

alyssawilk commented Jul 10, 2017 via email

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!

mattklein123 commented Jul 11, 2017

Uh oh!

htuch 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

alyssawilk Jul 12, 2017 •

edited

Loading

alyssawilk Jul 12, 2017 •

edited

Loading

alyssawilk Jul 12, 2017 •

edited

Loading