GH-36512: [C++][FlightRPC] Add async GetFlightInfo client call #36517

lidavidm · 2023-07-06T21:21:41Z

Rationale for this change

Async is a long-requested feature.

What changes are included in this PR?

Just the C++ implementation of async GetFlightInfo for the client.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes, new APIs.

Closes: [C++][FlightRPC] Async Flight: GetFlightInfo implementation #36512

lidavidm · 2023-07-06T21:23:47Z

Assuming the interface is roughly OK, I'd like to add basic Python bindings in the next PR (to make sure this interface adapts properly to asyncio/AnyIO)

lidavidm · 2023-07-07T00:18:46Z

Ah, I always forget the windows export macros...

cpp/src/arrow/flight/types.cc

cpp/src/arrow/flight/client.cc

cpp/src/arrow/flight/transport/grpc/grpc_client.cc

kou

+1

cpp/src/arrow/flight/client.cc

cpp/src/arrow/flight/test_definitions.cc

cpp/src/arrow/flight/transport/grpc/grpc_client.cc

cpp/src/arrow/flight/types.h

cpp/src/arrow/flight/client.h

cpp/src/arrow/flight/flight_test.cc

cpp/src/arrow/flight/client.h

cpp/src/arrow/flight/serialization_internal.h

cpp/src/arrow/flight/types_async.h

cpp/src/arrow/flight/transport.h

cpp/src/arrow/flight/transport/grpc/grpc_client.cc

cpp/src/arrow/flight/transport/grpc/util_internal.cc

lidavidm · 2023-07-17T18:01:20Z

TODOs:

Use Status instead of TransportStatus, add TransportStatusDetail
Replicate the crash and see if we can avoid it via a background destructor thread
Add overload to directly return a Future<FlightInfo> (and use that for convenience in tests)
Start looking into HTTP status codes as the canonical status codes (incl. how they play out with gRPC) (separate task)
It seems some versions of gRPC we build with don't support async yet, may have to ifdef things out (though, I need to rebase this again I think)
Debug MinGW

lidavidm · 2023-07-17T18:06:46Z

Ah, it may be experimental before v1.40... grpc/grpc@3e19bab

lidavidm · 2023-08-07T17:06:31Z

This has been ready for review for about 2 weeks.

lidavidm · 2023-08-07T18:41:24Z

It looks like another commit introduced clang-format errors.

kou · 2023-08-08T02:36:10Z

Sorry. #37048 fixed the lint error.

kou

Can we merge this because this is still an experimental API?
If we found a problem with this API later, we can fix it in follow-up tasks.

kou · 2023-08-08T02:43:22Z

cpp/src/arrow/flight/flight_internals_test.cc

@@ -742,5 +740,7 @@ TEST(TransportErrorHandling, ReconstructStatus) {
  ASSERT_EQ(detail->extra_info(), "Binary error details");
 }

+// TODO: test TransportStatusDetail


Should we resolve this TODO in this PR or can we defer to a follow-up PR?

I'd rather follow up in a separate PR since it appears the PR is already too large to get reviews

pitrou

Sorry for the delay, I was away. A couple of comments below but overall I agree it is good to go.

cpp/src/arrow/flight/test_definitions.cc

pitrou · 2023-08-08T13:30:36Z

cpp/src/arrow/flight/test_definitions.cc

+
+  ASSERT_FINISHES_OK(future);
+}
+


It would be nice to add a test that 1) starts a long-running async operation 2) closes the client 3) checks the Future is well-behaved (perhaps it returns Status::Cancelled?).

Perhaps in another PR?

We cannot handle closing the client, as explained in the comment below - something has to own the resources at some point that isn't the Future itself.

Fair enough, but I'll note that this sort of situation will come up easily in Python.

cpp/src/arrow/flight/test_definitions.cc

cpp/src/arrow/flight/transport/grpc/grpc_client.cc

pitrou

+1, let's go with this as this is already a nice addition.

kou · 2023-08-10T00:22:34Z

@lidavidm Could you update cpp/src/arrow/flight/transport/ucx/ucx_internal.cc as a follow-up task? It still uses internal::TransportStatusCode.

lidavidm · 2023-08-10T00:42:29Z

#37099

conbench-apache-arrow · 2023-08-12T22:55:46Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 9f183fc.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about possible false positives for unstable benchmarks that are known to sometimes produce them.

…pache#36517) ### Rationale for this change Async is a long-requested feature. ### What changes are included in this PR? Just the C++ implementation of async GetFlightInfo for the client. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes, new APIs. * Closes: apache#36512 Authored-by: David Li <[email protected]> Signed-off-by: David Li <[email protected]>

lidavidm mentioned this pull request Jul 6, 2023

GH-16604: [C++][FlightRPC] Add async Flight client #36205

Closed

github-actions bot added Component: C++ Component: FlightRPC awaiting committer review Awaiting committer review labels Jul 6, 2023

kou reviewed Jul 14, 2023

View reviewed changes

cpp/src/arrow/flight/types.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/client.cc Outdated Show resolved Hide resolved

cpp/src/arrow/flight/transport/grpc/grpc_client.cc Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Jul 14, 2023

kou approved these changes Jul 14, 2023

View reviewed changes

kou reviewed Jul 14, 2023

View reviewed changes

cpp/src/arrow/flight/types.h Show resolved Hide resolved

lidavidm force-pushed the flight-async-cpp-client-getflightinfo branch from d842802 to b2c4774 Compare July 14, 2023 18:10

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jul 14, 2023

pitrou reviewed Jul 17, 2023

View reviewed changes

cpp/src/arrow/flight/types.h Show resolved Hide resolved

pitrou reviewed Jul 17, 2023

View reviewed changes

cpp/src/arrow/flight/client.h Outdated Show resolved Hide resolved

lidavidm mentioned this pull request Jul 17, 2023

[FlightRPC] Explore using HTTP status codes as the canonical status definition #36722

Closed

pitrou reviewed Jul 17, 2023

View reviewed changes

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Jul 17, 2023

kou reviewed Aug 8, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 8, 2023

pitrou reviewed Aug 8, 2023

View reviewed changes

lidavidm added 3 commits August 8, 2023 10:27

apacheGH-36512: [C++][FlightRPC] Add async GetFlightInfo client call

bc9c33a

Update garbage bin

ea7fa4f

Feedback

e2738ac

lidavidm force-pushed the flight-async-cpp-client-getflightinfo branch from df885ea to e2738ac Compare August 8, 2023 14:27

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 8, 2023

Feedback (2)

ec50582

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels Aug 8, 2023

lidavidm requested a review from pitrou August 9, 2023 14:15

pitrou approved these changes Aug 9, 2023

View reviewed changes

lidavidm merged commit 9f183fc into apache:main Aug 9, 2023
35 of 36 checks passed

lidavidm removed the awaiting changes Awaiting changes label Aug 9, 2023

lidavidm deleted the flight-async-cpp-client-getflightinfo branch August 9, 2023 15:02

lidavidm mentioned this pull request Aug 10, 2023

[FlightRPC][C++] Update Flight-UCX to use TransportStatusCode #37099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-36512: [C++][FlightRPC] Add async GetFlightInfo client call #36517

GH-36512: [C++][FlightRPC] Add async GetFlightInfo client call #36517

lidavidm commented Jul 6, 2023 •

edited by github-actions bot

Loading

lidavidm commented Jul 6, 2023

lidavidm commented Jul 7, 2023

kou left a comment

lidavidm commented Jul 17, 2023 •

edited

Loading

lidavidm commented Jul 17, 2023

lidavidm commented Aug 7, 2023

lidavidm commented Aug 7, 2023

kou commented Aug 8, 2023

kou left a comment

kou Aug 8, 2023

lidavidm Aug 8, 2023

pitrou left a comment

pitrou Aug 8, 2023

lidavidm Aug 8, 2023

pitrou Aug 9, 2023

pitrou left a comment

kou commented Aug 10, 2023

lidavidm commented Aug 10, 2023

conbench-apache-arrow bot commented Aug 12, 2023

GH-36512: [C++][FlightRPC] Add async GetFlightInfo client call #36517

GH-36512: [C++][FlightRPC] Add async GetFlightInfo client call #36517

Conversation

lidavidm commented Jul 6, 2023 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

lidavidm commented Jul 6, 2023

lidavidm commented Jul 7, 2023

kou left a comment

Choose a reason for hiding this comment

lidavidm commented Jul 17, 2023 • edited Loading

lidavidm commented Jul 17, 2023

lidavidm commented Aug 7, 2023

lidavidm commented Aug 7, 2023

kou commented Aug 8, 2023

kou left a comment

Choose a reason for hiding this comment

kou Aug 8, 2023

Choose a reason for hiding this comment

lidavidm Aug 8, 2023

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

pitrou Aug 8, 2023

Choose a reason for hiding this comment

lidavidm Aug 8, 2023

Choose a reason for hiding this comment

pitrou Aug 9, 2023

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

kou commented Aug 10, 2023

lidavidm commented Aug 10, 2023

conbench-apache-arrow bot commented Aug 12, 2023

lidavidm commented Jul 6, 2023 •

edited by github-actions bot

Loading

lidavidm commented Jul 17, 2023 •

edited

Loading