Add DataStreamBidirectional API

[corbin-phipps]

Type

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

Side Effects

• Breaking change
• Non-functional change

Goals

This PR adds the DataStreamBidirectional API along with a unit test.

Technical Details

• Added DataStreamBidirectional API to NetRemoteDataStreamingService.proto.
• Added DataStreamReaderWriter implementation of the gRPC ServerBidiReactor to NetRemoteDataStreamingReactors.
• Added DataStreamReaderWriter implementation of the gRPC ClientBidiReactor to TestNetRemoteDataStreamingReactors.
• Added unit test to TestNetRemoteDataStreamingServiceClient.

Test Results

All tests pass.

Reviewer Focus

None.

Future Work

• Apply the work for Check when mismatched sequencing occurs in DataStreamDownload test client reactor #186 to this API as well.
• Check if access/writes to m_status need to be gated.
• Consider if the client will ever need to cancel this RPC and if so, add Cancel() function and another unit test.

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]

Are there plans to allow configuration of the block size?

[corbin-phipps]

Not currently. Because this API doesn't have a "request"/"response" like regular RPCs and instead has a "stream" for both directions, there is no intuitive way to allow the client to specify a block size that it wants the server to use. I think that if there was a scenario that required that, then the DataStreamDownload API should be used instead

[abeltrano]

If this is a requirement (I think it is), there are a few ways we could consider achieving this:

1. Since all fields are optional in proto3 format, the first input message from the client could populate a field that specifies properties about the requested stream from the server. This field is only inspected on the first message received and otherwise not even checked for.
2. Streaming could be stateful, where a request is first made to create a stream, and then that stream has to be explicitly started with the usual API. This way, both the client and server can negotiate stream properties. It could look something like this:

enum DataStreamDirection
{
    Upload,
    Download,
    Both,
};
enum DataStreamCreateStatusCode 
{
    Unknown = 0,
    Succeeded = 1,
    Failed = 2,
    // Others as needed
};
message DataStreamCreateRequest
{
    DataStreamDirection Direction = 1;
    DataStreamProperties PropertiesUpload = 2; // present when Direction == Upload and Both
    DataStreamProperties PropertiesDownload = 2; // present when Direction == Download and Both
};
message DataStreamCreateResult
{
    DataStreamCreateStatusCode Status = 0;
    string StreamId = 1; // Valid/present only when  Status == Succeeded
};
message DataStreamDownloadRequest
{
    string StreamId = 0;
};
rpc DataStreamCreate (DataStreamCreateRequest request, DataStreamCreateResult result);

The server doesn't strictly need upload stream properties since it's just a sink and will just drop everything it receives, however, that information would also enable the server to do accounting on the streaming operation. This could be useful in triaging failures in case the problem on the client side somehow precludes correct accounting, and, it would also be useful to be able to compare accounting on each side to make sure it matched.

[corbin-phipps]

Tracking this in #206 and will look into it later. Your first suggestion is definitely simpler and stays closer to the original design, but your second suggestion is cleaner and simple to understand, although requires a bit more work and changes the design a bit.

[abeltrano]

Thanks! Both require logic on the server side to check for the presence of a field, and while intuitive for the client, having conditional logic like that feels wrong. Anyway, we can drill into this as part of #206 after we determine if this is really a requirement of not.