GH-35500: [C++][Go][Java][FlightRPC] Add support for result set expiration #36009

kou · 2023-06-09T08:45:01Z

Rationale for this change

Currently, it is undefined whether a client can call DoGet more than once. Clients may want to retry requests, and servers may not want to persist a query result forever.

What changes are included in this PR?

Add an expiration time to FlightEndpoint. If present, clients may assume they can retry DoGet requests. Otherwise, clients should avoid retrying DoGet requests.

This is not a full retry protocol.

Also, add "pre-defined" actions to Flight RPC for working with result sets. These are pre-defined Protobuf messages with standardized encodings for use with DoAction:

CancelFlightInfo: Asynchronously cancel the execution of a distributed query. (Replaces the equivalent Flight SQL action.)
RefreshFlightEndpoint: Request an extension of the expiration of a FlightEndpoint.

This lets the ADBC/JDBC/ODBC drivers for Flight SQL explicitly manage result set lifetimes. These can be used with Flight SQL as regular actions.

Backward compatibility

Flight SQL's CancelQuery is deprecated by Flight RPC's CancelFlightInfo. But some clients may not be able to migrate to CancelFlightInfo entirely.

If a client can assume that a server requires 13.0.0 or later, the client can always use CancelFlightInfo. Otherwise, the client need to use CancelQuery (for old server) and/or CancelFlightInfo (for newer server).

A server needs to implement only CancelFlightInfo. Because Flight SQL server libraries provide the default CancelQuery implementation that delegates to CancelFlightInfo. Clients can use both of Flight SQL's CancelQuery and Flight RPC's CancelFLightInfo by this feature.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes.

Closes: [C++][Go][Java][FlightRPC] Add support for result set expiration #35500

github-actions · 2023-06-09T08:45:28Z

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

kou · 2023-06-09T08:48:14Z

It's not completed yet. See TODO list in the description.
The C++ implementation only exists for now.
I'll add the Go implementation and update documents then clean up the current C++ implementation.
I think that we need to consider how to keep backward compatibility in Flight SQL API.

lidavidm · 2023-06-09T14:07:58Z

dev/archery/archery/integration/runner.py

I can work on Java next week

format/Flight.proto

cpp/src/arrow/flight/serialization_internal.cc

cpp/src/arrow/flight/client.cc

cpp/src/arrow/flight/sql/client.h

kou · 2023-06-10T03:28:40Z

format/Flight.proto

We may want to remove Action:

Suggested change

message ActionCancelFlightInfoResult {

message CancelFlightInfoResult {

+1 from me too

OK. I've removed Action.

kou · 2023-06-10T03:30:55Z

format/Flight.proto

We may want to remove Cancel:

Suggested change

enum CancelResult {

enum Result {

Or we may want to move this to top-level and return this enum directly instead of wrapping by ActionCancelFlightInfoResult message.

At least in Protobuf/gRPC itself, you can only return messages and not bare enums

kou · 2023-06-10T03:33:18Z

format/Flight.proto

CANCEL_RESULT_ prefix may be redundant.

Suggested change

CANCEL_RESULT_UNSPECIFIED = 0;

UNSPECIFIED = 0;

We should use prefix.

https://protobuf.dev/programming-guides/style/#enums

Prefer prefixing enum values instead of surrounding them in an enclosing message.

Yes, the reason (in case you didn't find it) is that enums in Protobuf use C-style namespacing, which is carried over into generated code. So it will end up as ActionCancelFlightInfoResult::[CANCEL_RESULT_]UNSPECIFIED, and so if you ever need a second enum, you need to prefix the name.

I moved CancelResult to top-level to reduce ActionCancelFlightInfoResult::... prefix.
Is it OK?

Can we use CancelStatus or something instead of CancelResult to use Result in CancelResult and ActionCancelFlightInfoResult?

Ah, to avoid confusion with Arrow Result?

I guess Arrow also uses Status, so both are the same to me. If using CancelStatus is clearer to you, then it sounds good to me.

Ah, to avoid confusion with Arrow Result?

No. I just want to avoid Result duplication in CancelFlightInfoResult and CancelResult.

BTW, we have message Result in Flight.proto. It may confuse us with Arrow Result...

BTW, we have message Result in Flight.proto. It may confuse us with Arrow Result...

Yes, the Flight one actually happened to come first, but it's unfortunate...

No. I just want to avoid Result duplication in CancelFlightInfoResult and CancelResult.

Ok. I think status is also good then.

zeroshade

@kou would you like me contribute the Go implementation? Or do you have that in the works? I have the time to do so if you need / want the help.

zeroshade · 2023-06-13T15:09:30Z

format/Flight.proto

+1 from me too

kou · 2023-06-14T00:11:29Z

@zeroshade Thanks for the offer! I have the Go implementation in local and I just need to implement one more integration test to cover all cases implemented in the C++ implementation. I'll be able to complete it and push to this branch today. But the Go implementation is needed to be cleaned up. Please review and/or push improvements to this branch after I push the Go implementation.

kou · 2023-07-03T00:39:05Z

The vote carried: https://lists.apache.org/thread/no26s310qn3v0n5x830d50k598fh0pvr

I'll merge this.

conbench-apache-arrow · 2023-07-07T02:10:32Z

Conbench analyzed the 6 benchmark runs on commit 0b7bd74d.

There were 7 benchmark results indicating a performance regression:

Commit Run on arm64-m6g-linux-compute at 2023-07-03 02:19:06Z
- params=32768/10000, source=cpp-micro, suite=parquet-encoding-benchmark
Commit Run on arm64-t4g-linux-compute at 2023-07-03 02:27:32Z
- params=complex_expression/batch_size:100000/real_time, source=cpp-micro, suite=arrow-acero-project-benchmark
and 5 more (see the report linked below)

The full Conbench report has more details.

… expiration (apache#36009) ### Rationale for this change Currently, it is undefined whether a client can call DoGet more than once. Clients may want to retry requests, and servers may not want to persist a query result forever. ### What changes are included in this PR? Add an expiration time to FlightEndpoint. If present, clients may assume they can retry DoGet requests. Otherwise, clients should avoid retrying DoGet requests. This is not a full retry protocol. Also, add "pre-defined" actions to Flight RPC for working with result sets. These are pre-defined Protobuf messages with standardized encodings for use with DoAction: * CancelFlightInfo: Asynchronously cancel the execution of a distributed query. (Replaces the equivalent Flight SQL action.) * RefreshFlightEndpoint: Request an extension of the expiration of a FlightEndpoint. This lets the ADBC/JDBC/ODBC drivers for Flight SQL explicitly manage result set lifetimes. These can be used with Flight SQL as regular actions. #### Backward compatibility Flight SQL's CancelQuery is deprecated by Flight RPC's CancelFlightInfo. But some clients may not be able to migrate to CancelFlightInfo entirely. If a client can assume that a server requires 13.0.0 or later, the client can always use CancelFlightInfo. Otherwise, the client need to use CancelQuery (for old server) and/or CancelFlightInfo (for newer server). A server needs to implement only CancelFlightInfo. Because Flight SQL server libraries provide the default CancelQuery implementation that delegates to CancelFlightInfo. Clients can use both of Flight SQL's CancelQuery and Flight RPC's CancelFLightInfo by this feature. ### Are these changes tested? Yes. ### Are there any user-facing changes? Yes. * Closes: apache#35500 Authored-by: Sutou Kouhei <kou@clear-code.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>

kou requested review from assignUser, lidavidm and raulcd as code owners June 9, 2023 08:45

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

lidavidm reviewed Jun 9, 2023

View reviewed changes

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

kou commented Jun 10, 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 Jun 10, 2023

zeroshade reviewed Jun 13, 2023

View reviewed changes

format/Flight.proto Outdated

Copy link

Member

zeroshade Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from me too

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jun 13, 2023

github-actions bot added Component: Go awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jun 14, 2023

kou and others added 20 commits June 26, 2023 18:13

Remove expired checks

fa8f7d4

Remove needless checks

3e21487

Remove unused imports

7d4ca10

Add support for CancelQuery() method

1f26633

Remove unused variables

05bea5f

Refresh -> Renew

99d4618

Use RenewFlightInfoRequest

53152dc

Fix style

d0b44ae

Remove unused import

54a37e7

Java: refresh -> renew

b732b12

Fix build error

111ecb9

Add CancelFlightInfoRequest

e964103

Remove CloseFlightInfo

95295c5

Format

5acb080

Clean up

f0e3ace

java: Add CancelFlightInfoRequest

61f3666

java: Remove CloseFlightInfo

3affd5d

Remove unused function

18c96ff

Add ticket content to error message

b5ba62b

Increase expiration time

c2f2f05

kou force-pushed the flight-result-set-expiration branch from ec1d2ad to c2f2f05 Compare June 26, 2023 09:14

Follow name change

8c60daf

kou merged commit 0b7bd74 into apache:main Jul 3, 2023

kou deleted the flight-result-set-expiration branch July 3, 2023 00:39

kou removed the awaiting change review Awaiting change review label Jul 3, 2023

lidavidm mentioned this pull request Aug 10, 2023

GH-34865: [C++][Java][Flight RPC] Add Session management messages #34817

Merged

	message ActionCancelFlightInfoResult {
	message CancelFlightInfoResult {

GH-35500: [C++][Go][Java][FlightRPC] Add support for result set expiration #36009

GH-35500: [C++][Go][Java][FlightRPC] Add support for result set expiration #36009

Uh oh!

Conversation

kou commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Backward compatibility

Are these changes tested?

Are there any user-facing changes?

Uh oh!

github-actions bot commented Jun 9, 2023

Uh oh!

kou commented Jun 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeroshade left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou commented Jun 14, 2023

Uh oh!

kou commented Jul 3, 2023

Uh oh!

conbench-apache-arrow bot commented Jul 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kou commented Jun 9, 2023 •

edited

Loading

kou commented Jun 9, 2023 •

edited

Loading