GH-36155: [C++][Go][Java][FlightRPC] Add support for long-running queries #36946

kou · 2023-07-31T08:04:16Z

Rationale for this change

In Flight RPC, FlightInfo includes addresses of workers alongside result partition info. This lets clients fetch data directly from workers, in parallel or even distributed across multiple machines. But this also comes with tradeoffs.

Queries generally don't complete instantly (as much as we would like them to). So where can we put the 'query evaluation time'?

In GetFlightInfo: block and wait for the query to complete.
- Con: this is a long-running blocking call, which may fail or time out. Then when the client retries, the server has to redo all the work.
- Con: parts of the result may be ready before others, but the client can't do anything until everything is ready.
In DoGet: return a fixed number of partitions
- Con: this makes handling worker failures hard. Systems like Trino support fault-tolerant execution by replacing workers at runtime. But GetFlightInfo has already passed, so we can't notify the client of new workers.
- Con: we have to know or fix the partitioning up front.

Neither solution is optimal.

What changes are included in this PR?

We can address this by adding a retryable version of GetFlightInfo: PollFlightInfo(FlightDescriptor)

PollFlightInfo returns PollInfo:

message PollInfo {
  // The currently available results so far.
  FlightInfo info = 1;
  // The descriptor the client should use on the next try.
  // If unset, the query is complete.
  FlightDescriptor flight_descriptor = 2;
  // Query progress. Must be in [0.0, 1.0] but need not be
  // monotonic or nondecreasing. If unknown, do not set.
  optional double progress = 3;
  // Expiration time for this request. After this passes, the server
  // might not accept the retry descriptor anymore (and the query may
  // be cancelled). This may be updated on a call to PollFlightInfo.
  google.protobuf.Timestamp expiration_time = 4;
}

See the documentation changes for details of them:
http://crossbow.voltrondata.com/pr_docs/36946/format/Flight.html#downloading-data-by-running-a-heavy-query

Are these changes tested?

Yes.

This has C++, Go and Java implementations and an integration test with them.

Are there any user-facing changes?

Yes.

Closes: [C++][Go][Java][FlightRPC] Add support for long-running queries #36155

github-actions · 2023-07-31T08:04:55Z

⚠️ GitHub issue #36155 has been automatically assigned in GitHub to PR creator.

lidavidm

LGTM, thank you! I mostly left comments on wording. I can follow up with Java/Python later.

format/Flight.proto

cpp/src/arrow/flight/types.h

lidavidm · 2023-07-31T12:27:54Z

cpp/src/arrow/flight/middleware.h

@@ -57,6 +57,7 @@ enum class FlightMethod : char {
  DoAction = 7,
  ListActions = 8,
  DoExchange = 9,
+  PollFlightInfo = 10,


We'll have to update this in Python (I can put up a PR for that later)

We'll do this in another PR. Issue for this: #36954

docs/source/format/Flight.rst

lidavidm · 2023-07-31T15:09:02Z

Actually, I just realized we didn't add any of the previous proposals to Python, so I filed a separate task for that: #36954

lidavidm · 2023-07-31T20:42:41Z

Java: kou#14

zeroshade

LGTM just a style comment

Thanks much for doing the Go side of this!

go/arrow/internal/flight_integration/scenario.go

kou · 2023-07-31T21:20:35Z

Thanks for suggesting wordings! They are very helpful for me.
I've applied all of them!

I'll update the Go style later.

kou · 2023-08-01T08:25:33Z

@github-actions crossbow submit preview-docs

github-actions · 2023-08-01T08:27:59Z

Revision: 5d71b7d

Submitted crossbow builds: ursacomputing/crossbow @ actions-a21b2bffe3

Task	Status
preview-docs

kou · 2023-08-01T20:44:36Z

http://crossbow.voltrondata.com/pr_docs/36946/format/Flight.html#downloading-data-by-running-a-heavy-query

alamb

TLDR is I read this proposal and it makes sense to me. Thank you for working on this. I only reviewed the proto and text, not the implementations

I left some minor suggested wording changes that I believe increase the precision and clarity of language, but I don't think they are necessary

Nice work

alamb · 2023-08-08T10:51:04Z

format/Flight.proto

+/*
+ * The information to process a long-running query.
+ */
+message RetryInfo {


Minor Editorial point is that calling this "RetryInfo" is somewhat misleading in my mind because it implies to me that the query is being retried, when in reality it is that current execution status that is being retried. Maybe something like "PartialFlightInfo" or something might make it clearer

Thanks. It's a good point.
RetryInfo means info to retry polling but I can understand what you thought.

@lidavidm What do you think about this? My suggestion is PollInfo to align PollFlightInfo().

PollInfo sounds good.

OK. I've renamed to PollInfo.

format/Flight.proto

alamb · 2023-08-08T11:03:15Z

prior to merging this PR I think we need to do a formal vote on the change to the spec on the mailing list

kou

I left some minor suggested wording changes that I believe increase the precision and clarity of language, but I don't think they are necessary

Thanks! It's very helpful! I've merged your suggestions.

prior to merging this PR I think we need to do a formal vote on the change to the spec on the mailing list

Yes. You're right. I'll do it in this week or next:

https://lists.apache.org/thread/qcjpcw6m3p15wqxp6n6rqzlx01v1fl3v

Next:

I'll start a vote for this proposal after we reach a consensus
on this proposal.

kou · 2023-08-08T21:30:52Z

format/Flight.proto

+/*
+ * The information to process a long-running query.
+ */
+message RetryInfo {


Thanks. It's a good point.
RetryInfo means info to retry polling but I can understand what you thought.

@lidavidm What do you think about this? My suggestion is PollInfo to align PollFlightInfo().

…ries In Flight RPC, FlightInfo includes addresses of workers alongside result partition info. This lets clients fetch data directly from workers, in parallel or even distributed across multiple machines. But this also comes with tradeoffs. Queries generally don't complete instantly (as much as we would like them to). So where can we put the 'query evaluation time'? * In `GetFlightInfo`: block and wait for the query to complete. * Con: this is a long-running blocking call, which may fail or time out. Then when the client retries, the server has to redo all the work. * Con: parts of the result may be ready before others, but the client can't do anything until everything is ready. * In `DoGet`: return a fixed number of partitions * Con: this makes handling worker failures hard. Systems like Trino support fault-tolerant execution by replacing workers at runtime. But GetFlightInfo has already passed, so we can't notify the client of new workers. * Con: we have to know or fix the partitioning up front. Neither solution is optimal. We can address this by adding a retryable version of `GetFlightInfo`: `PollFlightInfo(FlightDescriptor)` `PollFlightInfo` returns `RetryInfo`: ```proto message RetryInfo { // The currently available results so far. FlightInfo info = 1; // The descriptor the client should use on the next try. // If unset, the query is complete. FlightDescriptor flight_descriptor = 2; // Query progress. Must be in [0.0, 1.0] but need not be // monotonic or nondecreasing. If unknown, do not set. optional double progress = 3; // Expiration time for this request. After this passes, the server // might not accept the retry descriptor anymore (and the query may // be cancelled). This may be updated on a call to PollFlightInfo. google.protobuf.Timestamp expiration_time = 4; } ``` See the documentation changes for details of them.

Co-authored-by: David Li <[email protected]>

Co-authored-by: Andrew Lamb <[email protected]>

kou · 2023-08-15T01:50:28Z

The discussion thread: https://lists.apache.org/thread/qcjpcw6m3p15wqxp6n6rqzlx01v1fl3v
The vote thread: https://lists.apache.org/thread/rwwqws6ghymx07qtz8d1jy5hsgljq96y
The vote carried: https://lists.apache.org/thread/9ty2vp3xnfnzcpl3wh5bnzbfszogomx2

conbench-apache-arrow · 2023-08-16T00:58:06Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 107b215.

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.

…ng queries (apache#36946) ### Rationale for this change In Flight RPC, FlightInfo includes addresses of workers alongside result partition info. This lets clients fetch data directly from workers, in parallel or even distributed across multiple machines. But this also comes with tradeoffs. Queries generally don't complete instantly (as much as we would like them to). So where can we put the 'query evaluation time'? * In `GetFlightInfo`: block and wait for the query to complete. * Con: this is a long-running blocking call, which may fail or time out. Then when the client retries, the server has to redo all the work. * Con: parts of the result may be ready before others, but the client can't do anything until everything is ready. * In `DoGet`: return a fixed number of partitions * Con: this makes handling worker failures hard. Systems like Trino support fault-tolerant execution by replacing workers at runtime. But GetFlightInfo has already passed, so we can't notify the client of new workers. * Con: we have to know or fix the partitioning up front. Neither solution is optimal. ### What changes are included in this PR? We can address this by adding a retryable version of `GetFlightInfo`: `PollFlightInfo(FlightDescriptor)` `PollFlightInfo` returns `PollInfo`: ```proto message PollInfo { // The currently available results so far. FlightInfo info = 1; // The descriptor the client should use on the next try. // If unset, the query is complete. FlightDescriptor flight_descriptor = 2; // Query progress. Must be in [0.0, 1.0] but need not be // monotonic or nondecreasing. If unknown, do not set. optional double progress = 3; // Expiration time for this request. After this passes, the server // might not accept the retry descriptor anymore (and the query may // be cancelled). This may be updated on a call to PollFlightInfo. google.protobuf.Timestamp expiration_time = 4; } ``` See the documentation changes for details of them: http://crossbow.voltrondata.com/pr_docs/36946/format/Flight.html#downloading-data-by-running-a-heavy-query ### Are these changes tested? Yes. This has C++, Go and Java implementations and an integration test with them. ### Are there any user-facing changes? Yes. * Closes: apache#36155 Lead-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Co-authored-by: David Li <[email protected]> Co-authored-by: Andrew Lamb <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>

kou requested review from assignUser, raulcd, zeroshade and lidavidm as code owners July 31, 2023 08:04

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

lidavidm approved these changes Jul 31, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Jul 31, 2023

github-actions bot added the Component: Java label Jul 31, 2023

zeroshade reviewed Jul 31, 2023

View reviewed changes

go/arrow/internal/flight_integration/scenario.go Outdated Show resolved Hide resolved

go/arrow/internal/flight_integration/scenario.go Outdated Show resolved Hide resolved

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

kou changed the title ~~GH-36155: [C++][Go][FlightRPC] Add support for long-running queries~~ GH-36155: [C++][Go][Java][FlightRPC] Add support for long-running queries Aug 1, 2023

kou force-pushed the flight-long-running-queries branch from 9fb0af3 to 5d71b7d Compare August 1, 2023 08:25

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

zeroshade approved these changes Aug 1, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Aug 1, 2023

alamb approved these changes Aug 8, 2023

View reviewed changes

kou commented Aug 8, 2023

View reviewed changes

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

kou force-pushed the flight-long-running-queries branch from b8b1d82 to 419d8cf Compare August 9, 2023 01:14

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

kou and others added 10 commits August 11, 2023 06:04

Add support for Protobuf < 3.15

3e5bb99

Add PollFlightInfo

424fe05

Use more "--experimental_allow_proto3_optional"

fca2c06

Add Java support

ddb1571

Fix wording

b26aea5

Co-authored-by: David Li <[email protected]>

Use switch

cec6890

Use POLL_FLIGHT_INFO

89f193f

Improve wording

d500eb9

Co-authored-by: Andrew Lamb <[email protected]>

RetryInfo -> PollInfo

ccbda9d

kou force-pushed the flight-long-running-queries branch from 419d8cf to ccbda9d Compare August 10, 2023 21:09

kou merged commit 107b215 into apache:main Aug 15, 2023

kou deleted the flight-long-running-queries branch August 15, 2023 01:51

kou removed the awaiting change review Awaiting change review label Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-36155: [C++][Go][Java][FlightRPC] Add support for long-running queries #36946

GH-36155: [C++][Go][Java][FlightRPC] Add support for long-running queries #36946

kou commented Jul 31, 2023 •

edited

Loading

github-actions bot commented Jul 31, 2023

lidavidm left a comment

lidavidm Jul 31, 2023

kou Aug 1, 2023

lidavidm commented Jul 31, 2023

lidavidm commented Jul 31, 2023

zeroshade left a comment •

edited

Loading

kou commented Jul 31, 2023

kou commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

kou commented Aug 1, 2023

alamb left a comment

alamb Aug 8, 2023

kou Aug 8, 2023

lidavidm Aug 8, 2023

kou Aug 9, 2023

alamb commented Aug 8, 2023

kou left a comment

kou Aug 8, 2023

kou commented Aug 15, 2023

conbench-apache-arrow bot commented Aug 16, 2023

GH-36155: [C++][Go][Java][FlightRPC] Add support for long-running queries #36946

GH-36155: [C++][Go][Java][FlightRPC] Add support for long-running queries #36946

Conversation

kou commented Jul 31, 2023 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jul 31, 2023

lidavidm left a comment

Choose a reason for hiding this comment

lidavidm Jul 31, 2023

Choose a reason for hiding this comment

kou Aug 1, 2023

Choose a reason for hiding this comment

lidavidm commented Jul 31, 2023

lidavidm commented Jul 31, 2023

zeroshade left a comment • edited Loading

Choose a reason for hiding this comment

kou commented Jul 31, 2023

kou commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

kou commented Aug 1, 2023

alamb left a comment

Choose a reason for hiding this comment

alamb Aug 8, 2023

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

kou Aug 9, 2023

Choose a reason for hiding this comment

alamb 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

kou commented Aug 15, 2023

conbench-apache-arrow bot commented Aug 16, 2023

kou commented Jul 31, 2023 •

edited

Loading

zeroshade left a comment •

edited

Loading