Adaptive Load library for calling Nighthawk Service

[eric846]

A library that calls a Nighthawk Service gRPC stub with the given CommandLineOptions, translating all possible gRPC errors into absl::StatusOr.

Will need to be updated when Nighthawk Service starts returning more than one message over the stream.

Part 4 of splitting PR #483.

[mum4k]

@dubious90 please review and merge once ready.

[dubious90]

Curious what you think of this train of thought:

At first I was going to ask if you could rename this to be Adaptive Load specific, since NighthawkServiceClient is the kind of name that could be used by a future class in Nighthawk, for creating a generic client for the NighthawkService.

Then I looked at the code, and I don't think there's anything adaptive-load specific about this class or the impl

[eric846]

You're right, moved it to common/.

[dubious90]

Can you add a line here indicating whether or not this class is thread-safe?

[eric846]

Added a comment to the impl.

[dubious90]

Not a request for change, but I wanted to hear your rationale: why have this as an argument on the function, rather than an argument to the class constructor?

It's interesting to me that neither this nor the impl version seem to hold any state at all, which leads to a question of why they need to be classes at all.

[eric846]

This used to be a bare function because it had no need to track state across calls. But in order to mock out this functionality for testing, with gtest it's much simpler to make a library export an interface rather than plain functions.

Leaving it stateless seems simpler than storing the stub and taking on responsibility for its lifetime. The intent here is definitely not to introduce a new layer into the system architecture, only to wrap a stateless helper function in a class for unit test purposes.

[dubious90]

Makes sense. Thanks for explaining.

[dubious90]

This seems like an odd api to me. Accepting a const command_line_options reference and then immediately copying it into a new object feels counterproductive.

There also doesn't seem to me to be a good reason to have duration as its own parameter as opposed to set on the command_line_options here, since the first and only thing you do with duration is set it in options. Could you accept a const reference to command line options, with the duration properly included, and then use that in ExecutionRequest?

[eric846]

This API has always bothered me a bit. It made more sense when it was a private helper and I was trying to save lines at the call sites. Changing this as you suggest.

[dubious90]

Hi Eric, mostly looks good. Just a couple comments.

[dubious90]

This one doesn't seem like an Unknown Error. Can we say this is an internal error? As currently, this is something that shouldn't happen and is entirely internal to the codebase

[eric846]

After looking at the docs, I found there is also Unavailable, meaning you to retry the request as-is. Unavailable might apply if the original Write fails since the Nighthawk Service might just have been down. But if you managed to Write but then got an error from WritesDone, Read, or Finish, those could all be Internal since they shouldn't happen. Does that make sense?

[dubious90]

Two small asks, but on each of these tests.

1. Can you add a linebreak between this EXPECT_CALL and the CommandLineOptions below (or in other tests, whatever is following it)? Just helps the eyes parse this a little as one block.

2. Can we comment at the top of the EXPECT_CALL statement roughly what this block is mocking? Or if that seems like a bad way to document this, something else that helps clarify for future readers what is happening.

[eric846]

Done