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

indigophox · 2023-03-31T02:55:35Z

Rationale for this change

Flight presently contains no formal mechanism for managing connection/query configuration options; instead, request headers and/or non-query SQL statements are often used in lieu, with unnecessary overhead and poor failure handling. A stateless (from Flight's perspective) Flight format extension is desirable to close this gap for server implementations that use/want connection state/context.

What changes are included in this PR?

"Session" set/get/close Actions and server-side helper middleware.

Are these changes tested?

Integration tests (C++ currently broken due to middleware-related framework issue) and some complex-case unit testing are included.

Are there any user-facing changes?

Non-breaking extensions to wire format and corresponding client/server Flight RPC API extensions.

Closes: [C++][FlightSQL] Add session management primitives #34865
GitHub Issue: [C++][FlightSQL] Add session management primitives #34865

github-actions · 2023-03-31T02:55:56Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

kou · 2023-03-31T06:52:47Z

Could you open a new issue for this pull request? This is not a MINOR change:

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)

* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)

lidavidm · 2023-03-31T11:37:05Z

I believe we discussed using headers as the base/fallback implementation; how does this integrate into that?

format/FlightSql.proto

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

format/FlightSql.proto

cpp/src/arrow/flight/types.cc

cpp/src/arrow/flight/types.h

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

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

cpp/src/arrow/flight/sql/server.cc

github-actions · 2023-04-03T17:21:54Z

Closes: [C++][FlightSQL] Add session management primitives #34865

github-actions · 2023-04-03T17:21:57Z

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

cpp/src/arrow/flight/types.h

cpp/src/arrow/flight/types.cc

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

cpp/src/arrow/flight/sql/server.cc

format/FlightSql.proto

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

format/FlightSql.proto

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

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

cpp/src/arrow/flight/sql/server_session_middleware.cc

…ers()

cpp/src/arrow/flight/integration_tests/test_integration.cc

kou · 2024-02-15T20:56:16Z

cpp/src/arrow/flight/integration_tests/test_integration.cc

+
+  arrow::Result<CloseSessionResult> CloseSession(
+      const ServerCallContext& context, const CloseSessionRequest& request) override {
+    // Broken (does not expire cookie) until C++ middleware SendingHeaders handling fixed.


Is this still broken?

Yes, pending #39791

Unfortunately nothing can be done about the issue here until the other fix is approved and merged.

I see. Could you add the issue number to the comment?

Done, pending a push so I don't spam the CI bot too much.

indigophox · 2024-02-15T21:08:30Z

Could you update the PR description?

Could you rebase on main to fix R related CI jobs?

Done and done. Kept the more recently PR description sections and removed the old text above them.

alamb

I reviewed the flightsql spec changes (not the C++/Java implementation) and I think they look well thought out and reasoned from my perspective.

We at InfluxData (primarily @kallisti-dev) are working on a concrete proposal for implementing prepared statements for stateless services -- #37720

While at first it might seem there is potential overlap with #37720 (e.g could we use cookies for #37720 or could this proposal encode session options in the handle), I believe since the session options target the entire connection rather than a specific statement, if makes sense to have two different mechanisms

So all in all this looks good to me. Thank you for your work @indigophox

lidavidm · 2024-02-20T13:45:14Z

I've closed the vote. I'll merge this now.

lidavidm · 2024-02-20T13:45:41Z

AppVeyor is a timeout; the Java pipeline is a flake

conbench-apache-arrow · 2024-02-20T20:24:46Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 2e2bd8b.

There were no benchmark performance regressions. 🎉

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

kou · 2024-02-22T22:05:02Z

@github-actions crossbow submit java-jars

github-actions · 2024-02-22T22:07:18Z

Revision: 265e5e8

Submitted crossbow builds: ursacomputing/crossbow @ actions-0f25a8e0a3

Task	Status
java-jars

…es (apache#34817) ### Rationale for this change Flight presently contains no formal mechanism for managing connection/query configuration options; instead, request headers and/or non-query SQL statements are often used in lieu, with unnecessary overhead and poor failure handling. A stateless (from Flight's perspective) Flight format extension is desirable to close this gap for server implementations that use/want connection state/context. ### What changes are included in this PR? "Session" set/get/close Actions and server-side helper middleware. ### Are these changes tested? Integration tests (C++ currently broken due to middleware-related framework issue) and some complex-case unit testing are included. ### Are there any user-facing changes? Non-breaking extensions to wire format and corresponding client/server Flight RPC API extensions. * Closes: apache#34865 Lead-authored-by: Paul Nienaber <paul.nienaber@dremio.com> Co-authored-by: Paul Nienaber <github@phox.ca> Co-authored-by: James Duong <jduong@dremio.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: David Li <li.davidm96@gmail.com> # Conflicts: # java/flight/flight-integration-tests/src/main/java/org/apache/arrow/flight/integration/tests/Scenarios.java # java/flight/flight-integration-tests/src/test/java/org/apache/arrow/flight/integration/tests/IntegrationTest.java # testing

…es (apache#34817) ### Rationale for this change Flight presently contains no formal mechanism for managing connection/query configuration options; instead, request headers and/or non-query SQL statements are often used in lieu, with unnecessary overhead and poor failure handling. A stateless (from Flight's perspective) Flight format extension is desirable to close this gap for server implementations that use/want connection state/context. ### What changes are included in this PR? "Session" set/get/close Actions and server-side helper middleware. ### Are these changes tested? Integration tests (C++ currently broken due to middleware-related framework issue) and some complex-case unit testing are included. ### Are there any user-facing changes? Non-breaking extensions to wire format and corresponding client/server Flight RPC API extensions. * Closes: apache#34865 Lead-authored-by: Paul Nienaber <paul.nienaber@dremio.com> Co-authored-by: Paul Nienaber <github@phox.ca> Co-authored-by: James Duong <jduong@dremio.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: David Li <li.davidm96@gmail.com>

indigophox requested a review from lidavidm as a code owner March 31, 2023 02:55

github-actions bot added Component: C++ Component: FlightRPC awaiting review Awaiting review labels Mar 31, 2023

lidavidm reviewed Mar 31, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Mar 31, 2023

indigophox changed the title ~~Add Session management messages, Location URI path accessors~~ GH-34865: [C++][Flight RPC] Add Session management messages, Location URI path accessors Apr 3, 2023

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 Apr 4, 2023

lidavidm reviewed Apr 17, 2023

View reviewed changes

lidavidm mentioned this pull request Apr 30, 2023

[C++][FlightRPC] Add support for using HTTP headers or query parameters in ServerAuthHandler #35373

Closed

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 23, 2023

lidavidm reviewed Jun 28, 2023

View reviewed changes

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

indigophox changed the title ~~GH-34865: [C++][Flight RPC] Add Session management messages, Location URI path accessors~~ GH-34865: [C++][Flight RPC] Add Session management messages Jul 4, 2023

github-actions bot removed the awaiting changes Awaiting changes label Jul 14, 2023

Paul Nienaber added 8 commits February 15, 2024 12:57

Docs touchup

07ac419

[Builds & passes] naming consistency fix

270c152

[Builds & passes] Docstring fixups

a1d19dc

Address PR feedback

33d66c4

Update Protobuf enum comments

591281a

Linter fixups & rebase

58381ba

Factor out addMiddlewareHeaders

042a193

Refactor GrpcServiceHandler::GetFlightInfo() to use addMiddlewareHead…

60c1298

…ers()

kou reviewed Feb 15, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Feb 15, 2024

PR feedback

f80b941

indigophox force-pushed the flight-sql-session-management branch from 986b599 to f80b941 Compare February 15, 2024 21:07

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Feb 15, 2024

PR feedback

265e5e8

alamb reviewed Feb 16, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Feb 16, 2024

lidavidm merged commit 2e2bd8b into apache:main Feb 20, 2024
41 of 43 checks passed

lidavidm removed the awaiting changes Awaiting changes label Feb 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

indigophox commented Mar 31, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Mar 31, 2023

kou commented Mar 31, 2023

lidavidm commented Mar 31, 2023

github-actions bot commented Apr 3, 2023

github-actions bot commented Apr 3, 2023

kou Feb 15, 2024

indigophox Feb 15, 2024

kou Feb 15, 2024

indigophox Feb 15, 2024

indigophox commented Feb 15, 2024

alamb left a comment

lidavidm commented Feb 20, 2024

lidavidm commented Feb 20, 2024

conbench-apache-arrow bot commented Feb 20, 2024

kou commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

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

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

Conversation

indigophox commented Mar 31, 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?

github-actions bot commented Mar 31, 2023

kou commented Mar 31, 2023

lidavidm commented Mar 31, 2023

github-actions bot commented Apr 3, 2023

github-actions bot commented Apr 3, 2023

kou Feb 15, 2024

Choose a reason for hiding this comment

indigophox Feb 15, 2024

Choose a reason for hiding this comment

kou Feb 15, 2024

Choose a reason for hiding this comment

indigophox Feb 15, 2024

Choose a reason for hiding this comment

indigophox commented Feb 15, 2024

alamb left a comment

Choose a reason for hiding this comment

lidavidm commented Feb 20, 2024

lidavidm commented Feb 20, 2024

conbench-apache-arrow bot commented Feb 20, 2024

kou commented Feb 22, 2024

github-actions bot commented Feb 22, 2024

indigophox commented Mar 31, 2023 •

edited by github-actions bot

Loading