grpc: implement BufferedAsyncClient for bidirectional gRPC stream by Shikugawa · Pull Request #18129 · envoyproxy/envoy

Shikugawa · 2021-09-15T07:16:43Z

Signed-off-by: Shikugawa [email protected]

Commit Message: grpc: implement BufferedAsyncClient for bidirectional gRPC stream
Additional Description: This function is a component for buffering and tracking the state of messages sent in the gRPC bidirectional stream. This component makes it possible to monitor and control the transmission status of highly granular messages from the Envoy side, and is identical to the one implemented in #17486.
Risk Level: Low
Testing: Unit
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

cc @snowp @lizan

Signed-off-by: Shikugawa <[email protected]>

Shikugawa · 2021-09-15T07:17:06Z

/assign-from @envoyproxy/envoy-maintainers

repokitteh-read-only · 2021-09-15T07:17:09Z

@envoyproxy/envoy-maintainers assignee is @dio

🐱

Caused by: a #18129 (comment) was created by @Shikugawa.

see: more, trace.

Signed-off-by: Shikugawa <[email protected]>

source/common/grpc/buffered_async_client.h

Signed-off-by: Shikugawa <[email protected]>

source/common/grpc/buffered_async_client.h

mathetake

Overall I would like to see the doc comment on the class.

source/common/grpc/buffered_async_client.h

Signed-off-by: Shikugawa <[email protected]>

…red-grpc-client Signed-off-by: Shikugawa <[email protected]>

Signed-off-by: Shikugawa <[email protected]>

Shikugawa · 2021-09-30T03:49:18Z

@lizan Fixed not to expose publishId(). Could you take a look?

mattklein123 · 2021-10-06T16:34:48Z

ping @lizan PTAL

source/common/grpc/buffered_async_client.h

Signed-off-by: Shikugawa <[email protected]>

…red-grpc-client Signed-off-by: Shikugawa <[email protected]>

Signed-off-by: Shikugawa <[email protected]>

davinci26 · 2021-10-12T18:24:42Z

/retest

repokitteh-read-only · 2021-10-12T18:24:45Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18129 (comment) was created by @davinci26.

see: more, trace.

Shikugawa · 2021-10-13T06:48:15Z

/retest

repokitteh-read-only · 2021-10-13T06:48:19Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #18129 (comment) was created by @Shikugawa.

see: more, trace.

Shikugawa · 2021-10-13T08:43:28Z

/retest

repokitteh-read-only · 2021-10-13T08:43:32Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #18129 (comment) was created by @Shikugawa.

see: more, trace.

Signed-off-by: Shikugawa <[email protected]>

repokitteh-read-only · 2021-10-15T07:30:10Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18129 (comment) was created by @Shikugawa.

see: more, trace.

Shikugawa · 2021-10-15T07:30:14Z

/retest

repokitteh-read-only · 2021-10-15T07:30:18Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18129 (comment) was created by @Shikugawa.

see: more, trace.

Signed-off-by: Shikugawa <[email protected]>

Shikugawa · 2021-11-02T08:45:29Z

@lizan friendly ping

lizan · 2021-11-02T19:37:58Z

/assign-from @envoyproxy/envoy-maintainers

for non-tetrands review

repokitteh-read-only · 2021-11-02T19:38:02Z

@envoyproxy/envoy-maintainers assignee is @zuercher

🐱

Caused by: a #18129 (comment) was created by @lizan.

see: more, trace.

zuercher

Just some questions about the API for the template. Let me know what you think.

source/common/grpc/buffered_async_client.h

zuercher · 2021-11-03T16:16:25Z

source/common/grpc/buffered_async_client.h

+    }
+
+    auto id = publishId();
+    message_buffer_[id] = std::make_pair(BufferState::Buffered, message);


In #17486 this id is set on the RequestType (CriticalAccessLogsMessage). In conjunction with the previous comment, maybe we should return absl::optional<uint64_t> with the id if buffered or null_opt if the buffer's full? The caller can then add the id to the message (although that kind of change via the message reference feels a little weird). Another possibility is to require that the id is passed obtained via publishId, used to call set_id on the message, and then passed to bufferMessage as a separate parameter.

I missed it. I agree with to return absl::optional<uint64_t> them the caller set it to the message.

Signed-off-by: Shikugawa <[email protected]>

zuercher

Thanks. I had some final comments, but otherwise looks good.

zuercher · 2021-11-05T17:09:47Z

source/common/grpc/buffered_async_client.h

+      auto& message = it.second.second;
+
+      if (state == BufferState::PendingFlush) {
+        continue;


I think we should add a test that covers this case, since it prevents double sending. I noticed it's not hit in the coverage report

source/common/grpc/buffered_async_client.h

zuercher · 2021-11-05T17:18:24Z

source/common/grpc/buffered_async_client.h

+    }
+  }
+
+  absl::optional<uint64_t> bufferMessage(RequestType& message) {


nit: Consider adding a comment that indicates that null_opt means buffer full.

Signed-off-by: Shikugawa <[email protected]>

zuercher

Thanks!

Shikugawa · 2021-11-10T02:01:53Z

@lizan Ready to merge

* main: (71 commits) bazel: fix macOS build (envoyproxy#18920) http: switching from 100 to 1xx (envoyproxy#18904) grpc: implement BufferedAsyncClient for bidirectional gRPC stream (envoyproxy#18129) bazel: add repository arg to benchmark_test (envoyproxy#18795) rocketmq_proxy: Improvement for map find (envoyproxy#18909) tls: unit test: spiffe signed by intermediate cert (envoyproxy#18914) Test for FilterConfigPerRoute dtor called on worker thread. (envoyproxy#18927) deps: Bump `com_google_protobuf` -> 3.19.1 (envoyproxy#18930) deps: Bump `com_googlesource_code_re2` -> 2021-11-01 (envoyproxy#18933) cvescan: Move cvescan data to yaml (envoyproxy#18947) remove unnecessary file level not unimplemented hide annotation (envoyproxy#18924) test: moving echo test (envoyproxy#18938) test: fixing a test flake (envoyproxy#18899) deps: Revert pyparsing bump (envoyproxy#18946) deps: Bump `build_bazel_rules_apple` -> 0.32.0 (envoyproxy#18932) deps: Bump `com_github_bazelbuild_buildtools` -> 4.2.3 (envoyproxy#18931) build(deps): bump pycparser from 2.20 to 2.21 in /tools/dependency (envoyproxy#18936) quic: supporting connections with zero initial available streams (envoyproxy#18775) test: moving proxy proto (envoyproxy#18939) build(deps): bump pyparsing from 3.0.4 to 3.0.5 in /tools/dependency (envoyproxy#18937) ... Signed-off-by: Michael Puncel <[email protected]>

grpc: implement BufferedAsyncClient for bidi gRPC stream

a400410

Signed-off-by: Shikugawa <[email protected]>

repokitteh-read-only bot assigned dio Sep 15, 2021

Shikugawa requested review from lizan and snowp September 15, 2021 07:17

Shikugawa mentioned this pull request Sep 15, 2021

access logging: introduce critical ALS endpoint #17486

Closed

lizan assigned lizan and unassigned dio Sep 15, 2021

add write buf high watermark test

4885e81

Signed-off-by: Shikugawa <[email protected]>

lizan suggested changes Sep 16, 2021

View reviewed changes

source/common/grpc/buffered_async_client.h Outdated Show resolved Hide resolved

source/common/grpc/buffered_async_client.h Show resolved Hide resolved

fix

82bf7c3

Signed-off-by: Shikugawa <[email protected]>

lizan suggested changes Sep 20, 2021

View reviewed changes

source/common/grpc/buffered_async_client.h Outdated Show resolved Hide resolved

mathetake reviewed Sep 22, 2021

View reviewed changes

source/common/grpc/buffered_async_client.h Outdated Show resolved Hide resolved

source/common/grpc/buffered_async_client.h Outdated Show resolved Hide resolved

Shikugawa added 4 commits September 23, 2021 08:14

fix

84073b0

Signed-off-by: Shikugawa <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into buffe…

469a5ca

…red-grpc-client Signed-off-by: Shikugawa <[email protected]>

not to expose puslishId()

8bff3c4

Signed-off-by: Shikugawa <[email protected]>

more comment

c43a128

Signed-off-by: Shikugawa <[email protected]>

lizan suggested changes Oct 6, 2021

View reviewed changes

source/common/grpc/buffered_async_client.h Outdated Show resolved Hide resolved

source/common/grpc/buffered_async_client.h Outdated Show resolved Hide resolved

Shikugawa added 3 commits October 8, 2021 09:42

fix

680eff8

Signed-off-by: Shikugawa <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into buffe…

cad140b

…red-grpc-client Signed-off-by: Shikugawa <[email protected]>

fix

952122e

Signed-off-by: Shikugawa <[email protected]>

Kick CI

f00e891

Signed-off-by: Shikugawa <[email protected]>

lizan added the waiting label Oct 15, 2021

Shikugawa closed this Oct 15, 2021

Shikugawa reopened this Oct 15, 2021

test

10ec8c5

Signed-off-by: Shikugawa <[email protected]>

repokitteh-read-only bot removed the waiting label Nov 2, 2021

lizan previously approved these changes Nov 2, 2021

View reviewed changes

lizan removed their assignment Nov 2, 2021

repokitteh-read-only bot assigned zuercher Nov 2, 2021

zuercher reviewed Nov 3, 2021

View reviewed changes

fix

4acfe2d

Signed-off-by: Shikugawa <[email protected]>

Shikugawa dismissed lizan’s stale review via 4acfe2d November 5, 2021 04:05

fix

a473666

Signed-off-by: Shikugawa <[email protected]>

zuercher reviewed Nov 5, 2021

View reviewed changes

fix

a7df1b1

Signed-off-by: Shikugawa <[email protected]>

zuercher approved these changes Nov 9, 2021

View reviewed changes

lizan merged commit 6df4b00 into envoyproxy:main Nov 10, 2021

Comments

Conversation

Shikugawa commented Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shikugawa commented Sep 15, 2021

Uh oh!

repokitteh-read-only bot commented Sep 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mathetake left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Shikugawa commented Sep 30, 2021

Uh oh!

mattklein123 commented Oct 6, 2021

Uh oh!

Uh oh!

Uh oh!

davinci26 commented Oct 12, 2021

Uh oh!

repokitteh-read-only bot commented Oct 12, 2021

Uh oh!

Shikugawa commented Oct 13, 2021

Uh oh!

repokitteh-read-only bot commented Oct 13, 2021

Uh oh!

Shikugawa commented Oct 13, 2021

Uh oh!

repokitteh-read-only bot commented Oct 13, 2021

Uh oh!

repokitteh-read-only bot commented Oct 15, 2021

Uh oh!

Shikugawa commented Oct 15, 2021

Uh oh!

repokitteh-read-only bot commented Oct 15, 2021

Uh oh!

Shikugawa commented Nov 2, 2021

Uh oh!

lizan commented Nov 2, 2021

Uh oh!

repokitteh-read-only bot commented Nov 2, 2021

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zuercher Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

Shikugawa Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

zuercher Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

Shikugawa Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zuercher Nov 5, 2021

Choose a reason for hiding this comment

Uh oh!

Shikugawa Nov 9, 2021

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Shikugawa commented Nov 10, 2021

Uh oh!

Reviewers

Assignees

Shikugawa commented Sep 15, 2021 •

edited

Loading