buffer: Use WatermarkFactory to create most WatermarkBuffer instances by antoniovicente · Pull Request #14256 · envoyproxy/envoy

antoniovicente · 2020-12-03T06:26:36Z

Commit Message:
buffer: Use WatermarkFactory to create most WatermarkBuffer instances

Also add watermark methods to the Buffer::Instance interface.

Risk Level: low, refactoring no functional changes expected.
Testing: covered by existing tests
Docs Changes: n/a
Release Notes: n/a

Also add watermark methods to the Buffer::Instance interface. Signed-off-by: Antonio Vicente <avd@google.com>

alyssawilk

Thanks for driving consistency improvements here!

Couple of thoughts to get you started :-)

include/envoy/buffer/buffer.h

alyssawilk · 2020-12-03T14:59:20Z

include/envoy/buffer/buffer.h

 using InstancePtr = std::unique_ptr<Instance>;

+// Informational enum that hints at the buffer's intended use.
+enum class BufferType {


Can you comment on what the intended use of this is?

Added a comment. If you're curious, here is the first followup change that depends on this. The end goal is to use this in e2e tests for the fix to #11370

master...antoniovicente:e2e_h2_high_buffering__pre_shared_ptr_factory

ok, so the intended use is to avoid a bunch of nasty test changes and potentially nasty test maintenance issues by switching more buffers to mocks?

I'd like to avoid this if we can.
I wonder if we can reduce the use of the mock buffer factory, and then back this part out. It looks like it's only really "used" in the tcp proxy integration test so I wonder if we could use a normal buffer factory in the base integration test, fix up the tcp proxy test with some comments (it's not likely to change as frequently as the others) and then drop buffertype from this PR. WDYT?

I'm really not a fan of mock buffers, so no - I am not planning to increase mock buffer use. I removed the BufferType argument and made the necessary changes to fix broken tests, see 5b5a99a

The change to the default behavior of the mock watermark factory created by the mock dispatcher fixes a bunch of small tests that use MockConnection

include/envoy/buffer/buffer.h

source/common/buffer/buffer_impl.h

source/common/buffer/watermark_buffer.h

Signed-off-by: Antonio Vicente <avd@google.com>

…fer_factory Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente

Removed the buffer type argument and figured out how to fix all the tests. PTAL.

antoniovicente · 2020-12-07T23:46:47Z

include/envoy/buffer/buffer.h

 using InstancePtr = std::unique_ptr<Instance>;

+// Informational enum that hints at the buffer's intended use.
+enum class BufferType {


I'm really not a fan of mock buffers, so no - I am not planning to increase mock buffer use. I removed the BufferType argument and made the necessary changes to fix broken tests, see 5b5a99a

The change to the default behavior of the mock watermark factory created by the mock dispatcher fixes a bunch of small tests that use MockConnection

alyssawilk

Awesome, glad that works out.
All comments optional, so you can merge, or update and will LGTM again.

alyssawilk · 2020-12-08T15:21:44Z

include/envoy/buffer/buffer.h

+   */
+  virtual void setWatermarks(uint32_t watermark) PURE;
+  /**
+   * Returns the configured high watermark.


optional, call out the 0 for disabled here as well

alyssawilk · 2020-12-08T15:30:26Z

source/common/network/connection_impl.h

  Buffer::InstancePtr write_buffer_;
+  // Ensure that if the consumer of the data from this connection isn't
+  // consuming, that the connection eventually stops reading from the wire.
+  Buffer::InstancePtr read_buffer_;


optional, for these new pointers, worth calling out they're never non-null, or overkill?

alyssawilk · 2020-12-08T15:31:07Z

test/common/buffer/buffer_fuzz.cc


  std::string toString() const override { return std::string(data_.data() + start_, size_); }

+  void setWatermarks(uint32_t) override {


followup: maybe worth adding this to the fuzzer when we merge classes.

Added a TODO

alyssawilk · 2020-12-08T15:33:24Z

test/common/buffer/buffer_fuzz.cc

+  }
+  uint32_t highWatermark() const override { return 0; }
+  // Returns true if the high watermark callbacks have been called more recently
+  // than the low watermark callbacks.


optional: can remove this comment given it's an override and commneted in the base class.

Signed-off-by: Antonio Vicente <avd@google.com>

…fer_factory Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente

Thanks for the review!

antoniovicente · 2020-12-08T18:48:54Z

source/common/network/connection_impl.h

  Buffer::InstancePtr write_buffer_;
+  // Ensure that if the consumer of the data from this connection isn't
+  // consuming, that the connection eventually stops reading from the wire.
+  Buffer::InstancePtr read_buffer_;


antoniovicente · 2020-12-08T18:49:07Z

test/common/buffer/buffer_fuzz.cc


  std::string toString() const override { return std::string(data_.data() + start_, size_); }

+  void setWatermarks(uint32_t) override {


Added a TODO

antoniovicente · 2020-12-08T18:49:44Z

include/envoy/buffer/buffer.h

+   */
+  virtual void setWatermarks(uint32_t watermark) PURE;
+  /**
+   * Returns the configured high watermark.


antoniovicente · 2020-12-08T18:50:09Z

test/common/buffer/buffer_fuzz.cc

+  }
+  uint32_t highWatermark() const override { return 0; }
+  // Returns true if the high watermark callbacks have been called more recently
+  // than the low watermark callbacks.


alyssawilk

Awesome!

* master: buffer: Optimize the layout of Slices in Buffer::OwnedImpl by removing subclassing and storing slice info directly in the SliceDeque (envoyproxy#14282) gRPC client to be used by ext_proc filter (envoyproxy#14283) http2: Add integration tests for PRIORITY frame flood mitigation for upstream servers (envoyproxy#14328) event: touch watchdog before execution of each post callback and before deferred deletion (envoyproxy#14339) stale: more allowed ops (envoyproxy#14345) stale: more changes (envoyproxy#14344) test: TODO fixup making enable_half_close private envoyproxy#14330) event: Reduce potential for lock contention while executing dispatcher post callbacks. (envoyproxy#14289) stale: fix config (envoyproxy#14337) metrics service sink: generalize the sink and grpc streamer for external use (envoyproxy#13919) wasm: update V8 to v8.8.278.8. (envoyproxy#14298) repo: switch to actions based stale bot (envoyproxy#14335) buffer: Use WatermarkFactory to create most WatermarkBuffer instances (envoyproxy#14256) Signed-off-by: Michael Puncel <mpuncel@squareup.com>

buffer: Use WatermarkFactory to create most WatermarkBuffer instances

7b61dd5

Also add watermark methods to the Buffer::Instance interface. Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente requested a review from alyssawilk December 3, 2020 06:26

antoniovicente requested review from PiotrSikora, asraa, dio, ggreenway and lizan as code owners December 3, 2020 06:26

jmarantz assigned alyssawilk Dec 3, 2020

alyssawilk reviewed Dec 3, 2020

View reviewed changes

alyssawilk added the waiting label Dec 3, 2020

address review comments

df96525

Signed-off-by: Antonio Vicente <avd@google.com>

repokitteh-read-only bot removed the waiting label Dec 3, 2020

antoniovicente added 4 commits December 4, 2020 16:35

Merge remote-tracking branch 'upstream/master' into use_watermark_buf…

095228d

…fer_factory Signed-off-by: Antonio Vicente <avd@google.com>

Kick CI

cb73e07

Signed-off-by: Antonio Vicente <avd@google.com>

revert buffer_type argument and mock method rename

8c68c12

Signed-off-by: Antonio Vicente <avd@google.com>

fix tests

5b5a99a

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente commented Dec 7, 2020

View reviewed changes

alyssawilk previously approved these changes Dec 8, 2020

View reviewed changes

antoniovicente added 3 commits December 8, 2020 13:35

update comments

8be2847

Signed-off-by: Antonio Vicente <avd@google.com>

Merge remote-tracking branch 'upstream/master' into use_watermark_buf…

a16a68f

…fer_factory Signed-off-by: Antonio Vicente <avd@google.com>

fix spelling

9c66978

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente dismissed alyssawilk’s stale review via 9c66978 December 8, 2020 18:46

antoniovicente commented Dec 8, 2020

View reviewed changes

alyssawilk approved these changes Dec 8, 2020

View reviewed changes

antoniovicente merged commit 61d80ba into envoyproxy:master Dec 8, 2020


		std::string toString() const override { return std::string(data_.data() + start_, size_); }

		void setWatermarks(uint32_t) override {

Conversation

antoniovicente commented Dec 3, 2020

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants