GH-34532: [Java][FlightSQL] Change JDBC to handle multi-endpoints #38521

jduo · 2023-10-30T23:54:27Z

Rationale for this change

The Flight SQL JDBC Driver currently doesn't fetch at multiple endpoints correctly when the data is not at the same location as the original connection.

What changes are included in this PR?

Create new clients to connect to new locations in endpoints.
If no location is reported using the current connection.
Make ArrowFlightSqlClientHandler's builder's build() function to be idempodent.
Add functionality to clone ArrowFlightSqClientHandler's builder so that it can be used for temporary connections to locations returned by getFlightInfo().
Add utility classes in unit tests for constructing a distributed Flight SQL Server

Are these changes tested?

Yes.

Are there any user-facing changes?

The behavior for when there are reported endpoints from getFlightInfo is now fixed. However if users relied on the previous behavior of just getting streams from the same node, and their server only ever reported the original node, they may observe more Flight client connections opening and closing than before (since new connections get spawned for each partition that has at least one Location now).

Closes: [Java] JDBC Flight SQL driver should follow Flight RPC spec #34532
GitHub Issue: [Java] JDBC Flight SQL driver should follow Flight RPC spec #34532

jduo · 2023-10-31T18:49:14Z

This work is partially based off #13900.

jduo · 2023-11-03T02:07:20Z

@lidavidm

lidavidm · 2023-11-03T14:28:05Z

...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java

+          // Clone the builder and then set the new endpoint on it.
+          final URI endpointUri = endpoint.getLocations().get(0).getUri();
+          final Builder builderForEndpoint = new Builder(ArrowFlightSqlClientHandler.this.builder)
+              .withHost(endpointUri.getHost())
+              .withPort(endpointUri.getPort())
+              .withEncryption(endpointUri.getScheme().equals(LocationSchemes.GRPC_TLS));
+
+          final ArrowFlightSqlClientHandler endpointHandler = builderForEndpoint.build();
+          try {
+            endpoints.add(new CloseableEndpointStreamPair(
+                endpointHandler.sqlClient.getStream(endpoint.getTicket(),
+                    endpointHandler.getOptions()), endpointHandler.sqlClient));
+          } catch (Exception ex) {
+            AutoCloseables.close(endpointHandler);
+            throw ex;
+          }


Some future improvements for this would be to test each of the locations and use the first one that works; ignore locations with unknown URI schemes for forwards compatibility; fall back to the current client if none of the locations work. Could we file issue(s) for this?

Also, having a cache of clients instead of opening new ones every time (e.g. via https://github.com/ben-manes/caffeine)

Added #38573 and #38574 for these topics.

- Create new clients to connect to new locations in endpoints. - If no location is reported using the current connection. - Rework ArrowFlightSqlHandler.Builder to avoid changing its state during build() so that it can be re-used. - Add a copy constructor to ArrowFlightSqlHandler.Builder to make it easy to replicate connection settings when connecting to different locations. - Add unit test infrastructure for building a distributed FlightSql server

aiguofer · 2023-11-03T18:29:46Z

...jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/client/ArrowFlightSqlClientHandler.java

+    private Builder(Builder original) {
+      this.middlewareFactories.addAll(original.middlewareFactories);


I just want to verify, this would ensure the CookiesMiddleware gets copied over for the Endpoint clients, correct?

Ah, I didn't think about that, but yes, it should mean that the middleware gets shared between the clients. That might be good or bad depending on the server implementation but we can follow up if it turns out to be an issue.

A new CookieMiddlewareFactory is created for subsequent connections (same with the AuthMiddlewareFactory). This was intentional to give child connections a 'blank-slate' but should actually be configurable.

Added #38576 . This one can cause breakage for existing users that are relying on the same cookie/auth token from being sent so I'll pick it up next.

Awesome thanks! we depend on cookies to maintain state and expect the callOptions we set to go out with every request so that will be important for us!

conbench-apache-arrow · 2023-11-03T21:16:05Z

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

There were no benchmark performance regressions. 🎉

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

…ts (apache#38521) ### Rationale for this change The Flight SQL JDBC Driver currently doesn't fetch at multiple endpoints correctly when the data is not at the same location as the original connection. ### What changes are included in this PR? - Create new clients to connect to new locations in endpoints. - If no location is reported using the current connection. - Make ArrowFlightSqlClientHandler's builder's build() function to be idempodent. - Add functionality to clone ArrowFlightSqClientHandler's builder so that it can be used for temporary connections to locations returned by getFlightInfo(). - Add utility classes in unit tests for constructing a distributed Flight SQL Server ### Are these changes tested? Yes. ### Are there any user-facing changes? The behavior for when there are reported endpoints from getFlightInfo is now fixed. However if users relied on the previous behavior of just getting streams from the same node, and their server only ever reported the original node, they may observe more Flight client connections opening and closing than before (since new connections get spawned for each partition that has at least one Location now). * Closes: apache#34532 Authored-by: James Duong <james.duong@improving.com> Signed-off-by: David Li <li.davidm96@gmail.com>

github-actions bot added Component: Java awaiting review Awaiting review labels Oct 30, 2023

jduo force-pushed the 34532-jdbc-flight-endpoints branch from cfec8d0 to 16cd5b2 Compare October 31, 2023 18:37

jduo force-pushed the 34532-jdbc-flight-endpoints branch from 16cd5b2 to 3b8f7bd Compare November 3, 2023 02:03

jduo marked this pull request as ready for review November 3, 2023 02:07

jduo requested a review from lidavidm as a code owner November 3, 2023 02:07

lidavidm approved these changes Nov 3, 2023

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting review Awaiting review labels Nov 3, 2023

jduo force-pushed the 34532-jdbc-flight-endpoints branch from 3b8f7bd to 9a0bd79 Compare November 3, 2023 16:02

lidavidm merged commit 2fb7fd9 into apache:main Nov 3, 2023
14 checks passed

lidavidm removed the awaiting merge Awaiting merge label Nov 3, 2023

aiguofer reviewed Nov 3, 2023

View reviewed changes

github-actions bot added the awaiting committer review Awaiting committer review label Nov 3, 2023

jduo mentioned this pull request Nov 3, 2023

[Java][FlightRPC] Add options to JDBC to reuse credentials and cookies in getStream sub-clients #38576

Closed

jduo mentioned this pull request Nov 6, 2023

GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver #38461

Merged

prmoore77 added a commit to prmoore77/arrow that referenced this pull request Nov 6, 2023

Added new fields to new copy constructor (from: apache#38521)

1975786

jduo mentioned this pull request Nov 20, 2023

[Java] jdbc:arrow-flight-sql executeQuery behaves incorrectly #38785

Closed

xinyiZzz mentioned this pull request Mar 4, 2024

[Java] JDBC Flight SQL driver not connect to individual endpoints specified by GetFlightInfo #40336

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-34532: [Java][FlightSQL] Change JDBC to handle multi-endpoints #38521

GH-34532: [Java][FlightSQL] Change JDBC to handle multi-endpoints #38521

jduo commented Oct 30, 2023 •

edited by github-actions bot

Loading

jduo commented Oct 31, 2023

jduo commented Nov 3, 2023

lidavidm Nov 3, 2023

lidavidm Nov 3, 2023

jduo Nov 3, 2023

aiguofer Nov 3, 2023

lidavidm Nov 3, 2023

jduo Nov 3, 2023 •

edited

Loading

jduo Nov 3, 2023

aiguofer Nov 3, 2023

conbench-apache-arrow bot commented Nov 3, 2023

		private Builder(Builder original) {
		this.middlewareFactories.addAll(original.middlewareFactories);

GH-34532: [Java][FlightSQL] Change JDBC to handle multi-endpoints #38521

GH-34532: [Java][FlightSQL] Change JDBC to handle multi-endpoints #38521

Conversation

jduo commented Oct 30, 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?

jduo commented Oct 31, 2023

jduo commented Nov 3, 2023

lidavidm Nov 3, 2023

Choose a reason for hiding this comment

lidavidm Nov 3, 2023

Choose a reason for hiding this comment

jduo Nov 3, 2023

Choose a reason for hiding this comment

aiguofer Nov 3, 2023

Choose a reason for hiding this comment

lidavidm Nov 3, 2023

Choose a reason for hiding this comment

jduo Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

jduo Nov 3, 2023

Choose a reason for hiding this comment

aiguofer Nov 3, 2023

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Nov 3, 2023

jduo commented Oct 30, 2023 •

edited by github-actions bot

Loading

jduo Nov 3, 2023 •

edited

Loading