Add initial DataStreamDownload API implementation

[corbin-phipps]

Type

• Bug fix
• Feature addition
• Feature update
• Documentation
• Build Infrastructure

Side Effects

• Breaking change
• Non-functional change

Goals

This PR adds the initial implementation of the DataStreamDownload API, including the API itself, the foundational service implementation and reactor class, a test client reactor class, and a basic unit test.

Technical Details

• Added DataStreamDownload API to NetRemoteDataStreamingService.proto, along with applicable types into NetRemoteDataStream.proto.
• Added DataStreamWriter implementation of the gRPC ServerWriteReactor reactor class to NetRemoteDataStreamingReactors.
• Added test client implementation of the gRPC ClientReadReactor reactor class.
• Added basic unit test.
• Reverted smart pointer memory management back to new/delete.

Test Results

All tests pass.

Reviewer Focus

None.

Future Work

• Handle DataStreamPattern options from the client's DataStreamDownloadRequest.
• Add additional unit test for DataStreamTypeContinuous with DataStreamPatternConstant.
• Add a better mechanism for data creation.

Checklist

• Build target all compiles cleanly.
• clang-format and clang-tidy deltas produced no new output.
• Newly added functions include doxygen-style comment block.

[abeltrano]

We should de-dupe Properties from the oneof field names. Also, Type probably isn't needed either:

    oneof Properties
    {
        DataStreamFixedTypeProperties Fixed = 3;
        DataStreamContinuousTypeProperties Continuous = 4;
    }

[abeltrano]

This is accepting a copy of the string, and then I believe another copy is made when passed to set_message. It probably makes sense to accept a const std::string& here instead, which will avoid 1 of those copies.

Also, in general, when accepting a copy/value of an argument, it's usually a good idea to accept it as non-const so the function can use it however it likes, since it owns that memory.

[abeltrano]

I think we'll want to eventually use these test classes outside test code (and move them to a common lib at that time).

In that case, it's likely we'll want to make this timeout value configurable. Suggest to at least add a member to DataStreamMember and read the member here instead of using the constant. Completely fine if the member is just initialized to a constant (with this value) and no way is provided for configuring it for now.

[abeltrano]

Does this refer to the throughput pattern, for example, the timing/rate of the data flow? In that case, Constant == an uninterrupted flow of data, correct?

[corbin-phipps]

Right, Constant would be a steady pattern with each data message being the same number of bytes

[abeltrano]

Is Data only present/valid when Status == Succeeded? If so, suggest re-ordering these such that always-present/valid fields are first, then optional/conditional ones following.

[corbin-phipps]

Yes that's true. I can re-order the fields so that Status is first since that will always be present.

[abeltrano]

Any reason this can't be allocated on the stack instead? Eg.

DataStreamReader dataStreamReader{  client.get(), &request };
// ...
grpc::Status status = dataStreamReader.Await(&numberOfDataBlocksReceived, &operationStatus);

[corbin-phipps]

I believe it can be stack allocated. Will update

[abeltrano]

Looking at this now, would it be helpful to know when mismatched sequencing occurs? Might this allow us to help quantify the number of blocks dropped/lost?

[corbin-phipps]

That's a good idea. It would be easy to tell - basically move this check into OnReadDone. We'd need to decide if we want to stop reading and cancel the RPC as soon as the first sequencing mismatch occurs, or if we want to keep going and later count how many blocks were dropped.

[corbin-phipps]

Going to defer this until a bit later since it's test code - tracked by #186