Horizontal scaling: Distributor gRPC service + tests by oschaaf · Pull Request #669 · envoyproxy/nighthawk

oschaaf · 2021-04-01T11:05:10Z

Part of the horizontal scaling effort: add gRPC distributor service + tests.

This new gRPC service allows delegation of load test execution initiation messages
to a set of load generator services.

Signed-off-by: Otto van der Schaaf ovanders@redhat.com

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

dubious90

Some nits and comments, but in general looks good. @mum4k to approve/merge.

dubious90 · 2021-04-02T21:30:44Z

source/distributor/service_impl.cc

+
+namespace Nighthawk {
+
+::grpc::Status NighthawkDistributorServiceImpl::validateRequest(


can this be a private function in an anonymous namespace, rather than a private method object?

done in 6471b86

dubious90 · 2021-04-02T21:32:48Z

source/distributor/service_impl.cc

+
+::grpc::Status NighthawkDistributorServiceImpl::validateRequest(
+    const ::nighthawk::DistributedRequest& request) const {
+  auto& validation_visitor = Envoy::ProtobufMessage::getStrictValidationVisitor();


nit: I think this would be more readable with ValidationVisitor& here instead of auto&. The type inference isn't quite obvious enough for me.

done in 6471b86

dubious90 · 2021-04-02T21:36:39Z

source/distributor/service_impl.cc

+    if (status.ok()) {
+      std::tuple<grpc::Status, nighthawk::DistributedResponse> status_and_response =
+          handleRequest(request);
+      status = std::get<0>(status_and_response);


if we know status = ok, this line has no purpose.

Well this does break the while loop when status is not OK?

dubious90 · 2021-04-02T21:42:50Z

source/distributor/service_impl.cc

+    const envoy::config::core::v3::Address& service,
+    const ::nighthawk::client::ExecutionRequest& request) const {
+  RELEASE_ASSERT(service_client_ != nullptr, "service_client_ != nullptr");
+  std::unique_ptr<nighthawk::client::NighthawkService::Stub> stub;


can we instead declare this where it's used?

done in 6471b86

dubious90 · 2021-04-02T21:43:56Z

source/distributor/service_impl.cc

+    const ::nighthawk::client::ExecutionRequest& request) const {
+  RELEASE_ASSERT(service_client_ != nullptr, "service_client_ != nullptr");
+  std::unique_ptr<nighthawk::client::NighthawkService::Stub> stub;
+  std::shared_ptr<::grpc::Channel> channel;


nit: remove :: at start of grpc::Channel

fixed where applicable in 6471b86

dubious90 · 2021-04-02T21:46:16Z

api/distributor/distributor.proto

  repeated envoy.config.core.v3.Address services = 3 [(validate.rules).repeated .min_items = 1];
 }

+message DistributedResponseFragment {


can we add a top-level explanation of the purpose of this object?

Also, looking at the code, I think that the intention here is that one of these is associated with every nighthawk service, and essentially operates as either:

We failed to connect to this service

Here's the service address.

Given that, is DistributedResponseFragment a good name here? I don't love the name fragment here, because it implies to me that they are parts of a whole (often without independent meaning outside of that whole).

Yes this is spot on; I'll think about a better name here. Also open to suggestions.

The DistributorRequest references services, so for a name how about DistributedServiceResponse or just DistributedResponse?

I think you were using Fragment to indicate that it's not the full resultset, but I don't think that's necessary here, as long as we document the intent.

(Some APIs I have used in the past have referred to this as an AsyncResponse, but I'm not sure how conventional that is).

Renamed in 5462844. I went with DistributedServiceResponse, is DistributedResponse was already used in:

message DistributedResponse { repeated DistributedServiceResponse service_response = 1; }

mum4k

Just started the review and then noticed that there are still outstanding comments from @dubious90, so the code will change.

Sending what I have and will continue once the existing comments are addressed. But please let me know if I misunderstood and this is blocked on me.

source/distributor/service_impl.h

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

…distributor-service Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

…distributor-service Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

mum4k

Looks good, just one question about asserts.

source/distributor/service_impl.cc

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Part of the horizontal scaling effort: add gRPC distributor service + tests. This new gRPC service allows delegation of load test execution initiation messages to a set of load generator services. Signed-off-by: Otto van der Schaaf <ovanders@redhat.com> Signed-off-by: William Juan <66322422+wjuan-AFK@users.noreply.github.com>

Otto van der Schaaf added 5 commits April 1, 2021 13:03

save state

59c633a

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Add two more tests w/ MockNighthawkServiceClient

c478e8f

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Test service write failure, clean up

d6edb7b

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Fix clang-tidy, tweak tests

83ab7de

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Use gRPC validation plugin, clean up

f133d7a

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

oschaaf changed the title ~~[WIP] Distributor gRPC service + tests~~ Distributor gRPC service + tests Apr 1, 2021

oschaaf marked this pull request as ready for review April 1, 2021 22:35

oschaaf added the waiting-for-review A PR waiting for a review. label Apr 1, 2021

oschaaf changed the title ~~Distributor gRPC service + tests~~ Horizontal scaling: Distributor gRPC service + tests Apr 1, 2021

dubious90 previously approved these changes Apr 2, 2021

View reviewed changes

dubious90 assigned mum4k Apr 2, 2021

mum4k requested changes Apr 5, 2021

View reviewed changes

source/distributor/service_impl.h Outdated Show resolved Hide resolved

source/distributor/service_impl.h Outdated Show resolved Hide resolved

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Apr 5, 2021

Partially address review feedback.

6471b86

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

oschaaf dismissed dubious90’s stale review via 6471b86 April 6, 2021 09:02

Otto van der Schaaf added 4 commits April 6, 2021 19:16

Merge remote-tracking branch 'upstream/main' into horizontal-scaling-…

cd4f82a

…distributor-service Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Remove pragma diagnostics: no longer needed

f3bc7a7

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

Merge remote-tracking branch 'upstream/main' into horizontal-scaling-…

02038a0

…distributor-service Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

s/DistributedResponseFragment/DistributedServiceResponse/

5462844

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Apr 20, 2021

mum4k requested changes Apr 20, 2021

View reviewed changes

source/distributor/service_impl.cc Outdated Show resolved Hide resolved

mum4k added waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. and removed waiting-for-review A PR waiting for a review. labels Apr 20, 2021

oschaaf added waiting-for-review A PR waiting for a review. and removed waiting-for-changes A PR waiting for comments to be resolved and changes to be applied. labels Apr 21, 2021

review feedback: Remove asserts

d0fb1b4

Signed-off-by: Otto van der Schaaf <ovanders@redhat.com>

mum4k approved these changes Apr 21, 2021

View reviewed changes

mum4k merged commit fe8e3a0 into envoyproxy:main Apr 21, 2021


		namespace Nighthawk {

		::grpc::Status NighthawkDistributorServiceImpl::validateRequest(

Conversation

oschaaf commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mum4k left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oschaaf commented Apr 1, 2021 •

edited

Loading