ARROW-15282: [C++][FlightRPC] Split data methods from the underlying transport

[lidavidm]

Defines interfaces to add new network transports to Arrow Flight, and rewrite the gRPC implementation in terms of these interfaces. The bulk of the new APIs are in transport.h. Where possible, code has been factored out so that transports have to implement as little as possible.

[lidavidm]

TODOs

• Address new TODOs scattered in source
• Make client/server share code around writing IPC with app metadata
• Document the new interfaces
• Check that Flight benchmark did not regress
• Check that CUDA support did not regress

[github-actions]

https://issues.apache.org/jira/browse/ARROW-15282

[lidavidm]

CC @cyb70289 if you have a chance to take a look (no pressure - this is a pretty large PR)

[cyb70289]

Looks great ! Some nits in comments.

Will test the PR at my side. Thanks.

[cyb70289]

LGTM.

On my test machine, DoPutLargeBatch often costs 10s to finish. It's not related to this PR. I've filed a jira issue: https://issues.apache.org/jira/browse/ARROW-15793

[lidavidm]

Thanks for tracking that down to the original PR. Removing the reset() fixes things. I also noticed FlightClient::Close() wasn't bound to ClientTransportImpl::Close() so that has also been fixed.

[pitrou]

The responsibilities on the server side are difficult to understand. It looks like they are shared between 3 or 4 different classes (FlightServerBase, ServerTransportImpl, FlightServiceImpl and perhaps FlightService::Service). I was expecting one transport-agnostic class and one transport implementation, but it looks like there is more...

[pitrou]

Also, from a file organization standpoint, I would expect the gRPC-specific parts to be in their own files and named perhaps grpc_{client,server}.{h,cc}?

[lidavidm]

I can try to document it better but effectively the layers are:

• FlightServerBase: application-facing API that provides overridable methods
• ServerTransportImpl: transport-facing API that provides an appropriate I/O stream implementation for a given method, and sets up the actual underlying server. Calls into FlightServiceImpl/FlightServerBase
• FlightServiceImpl: implements application-facing APIs in terms of the transport-facing APIs (effectively, code that is supposed to be transport- and application- agnostic, i.e. "Flight itself")

Ideally transports wouldn't deal with FlightServerBase but I don't want to go through and wrap all the non-data methods quite yet. But effectively, a transport only implements ServerTransportImpl. It's just that the gRPC code is still with the rest of the Flight code; would it be easier if I went ahead and split them out?

FlightService::Serivce/FlightGrpcServiceImpl are gRPC specific

[lidavidm]

Also, from a file organization standpoint, I would expect the gRPC-specific parts to be in their own files and named perhaps grpc_{client,server}.{h,cc}?

Ok, that should be doable

[lidavidm]

I should also add a doc page for transports (and the Flight documentation is generally lacking; I should take the time to clean things up)

[cyb70289]

I promise this is my last comment :)
There are some legacy comments about grpc found by git grep gRPC {client,server}.{h,cc}.

[lidavidm]

Thanks, I thought I checked but apparently not!

[ursabot]

Benchmark runs are scheduled for baseline = dfca6a7 and contender = bb65225. bb65225 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️1.97% ⬆️1.8%] test-mac-arm
[Finished ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.77% ⬆️0.17%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

[lidavidm]

Thank you both for the reviews!