ARROW-15921: [Format][FlightRPC][C++][Java] Clarify interpretation of FlightEndpoint.locations #12636

lidavidm · 2022-03-15T15:36:35Z

Clarify that an empty list of locations means we should fetch data from the same server, and that multiple locations mean that all locations are equivalent.

See "[Java] [Flight] Usage of Locations on FlightEndpoint" on user@ for the original discussion.

…erpretation

…ions

…tions

github-actions · 2022-03-15T16:13:17Z

https://issues.apache.org/jira/browse/ARROW-15921

BryanCutler

LGTM, just had a minor question

BryanCutler · 2022-03-15T21:57:39Z

format/Flight.proto

+   * If the list is not empty, the expectation is that the ticket can
+   * be redeemed at any of the locations, and that the data returned
+   * will be equivalent. In other words, multiple locations provide
+   * redundancy/load balancing.


Is there an intention that if the endpoint list is not empty, the ticket must be redeemed from one of those and not the service that generated the ticket, or is that up to interpretation by the application?

Ah, this is ambiguous still, but I think the former was intended. For instance from the original Flight blog post:

Nodes in a distributed cluster can take on different roles. For example, a subset of nodes might be responsible for planning queries while other nodes exclusively fulfill data stream (DoGet or DoPut) requests.

@jacques-n what do you think?

I think the reason the original might be thought of as ambiguous is that people are failing to see that tickets were conceived of as single use. If tickets are single use, once I go to one location and use the ticket, I can't then use the ticket at a second location. A flight must be a consumption of all tickets to be complete. I wonder if instead of expressing here, we should clarify elsewhere that a ticket is single use and reuse of ticket results in undefined behavior. Thoughts?

I think that would be good to clarify, but that doesn't answer the question of, "if there are locations, can the ticket still be redeemed on the original server, or must it only be redeemed on one of the given locations"?

Yeah. The intention was, if there are locations, you must use one of the locations to redeem the ticket. One of the locations may be the current server.

Thanks for the comments, I've updated this PR with both the Ticket and locations clarifications

That sounds reasonable to me

...ion-tests/src/main/java/org/apache/arrow/flight/integration/tests/IntegrationTestClient.java

BryanCutler

I just had a thought on clarifying a little more about multiple endpoints, but otherwise looks good

BryanCutler · 2022-03-17T20:56:52Z

format/Flight.proto

+   * A list of endpoints associated with the flight. To consume the
+   * whole flight, all endpoints (and hence all Tickets) must be consumed.
+   *
+   * In other words, multiple endpoints provide partitioning.


Sorry to come back to this, but I've had people come to me thinking that Flight will do the partitioning for them. So we don't want to make it seem like that is what's happening. Maybe reword slightly to something like "In other words, multiple endpoints can be used with partitioned data." or just "... multiple endpoints provide for partitioned data." ?

That's a very good point. I've reworded things here, how does it look now?

I'm confused by your comment here a little bit. Flight can do partitioning. It is done by returning more than one ticket for a particular flight. (I agree that locations aren't about partitioning.)

I suppose it's because an application can implement partitioning via Flight, but Bryan has seen people thinking this means that using Flight will automatically partition/distribute data.

What I meant was that Flight doesn't have built-in logic to actively partition a given data set. It's up to the service implementation to assign partitions to a given ticket.

jduo · 2022-03-17T21:55:27Z

format/Flight.proto

-   * A list of endpoints associated with the flight. To consume the whole
-   * flight, all endpoints must be consumed.
+   * A list of endpoints associated with the flight. To consume the
+   * whole flight, all endpoints (and hence all Tickets) must be consumed.


Is it worth clarifying that the list of endpoints can be consumed in any order?

Updated.

Though - that brings up the question of how to represent an ordered dataset. Is the expectation that (say) if results are sorted, then the endpoints should also be sorted?

My thought here is that the backend should enforce ordering by grouping ordered data together under one ticket. The client can't process this data in parallel anyway.

This would prevent exposing ordered fragments from separate locations though. It sort of makes sense for a query engine to have to collate data under a single node though.

That makes sense. Though I think this is getting far out of scope of the original clarification now.

I agree with @jduo on this comment. If data must be ordered, at least for now, it should be exposed via single ticket/endpoint.

Pushed a short clarification.

jacques-n

+1 for the proto changes. I didn't review the code changes.

lidavidm · 2022-03-18T18:29:55Z

@emkornfield, @pitrou: any thoughts on the Flight spec clarifications here?

pitrou

The clarification is very useful, thank you. Just two comments.

pitrou · 2022-03-21T13:43:29Z

format/Flight.proto

-   * A list of URIs where this ticket can be redeemed. If the list is
-   * empty, the expectation is that the ticket can only be redeemed on the
-   * current service where the ticket was generated.
+   * A list of URIs where this ticket can be redeemed.


Perhaps also explain how someone is supposed to "redeem" a ticket (note that verb doesn't seem to exist in the RPC APIs)?

pitrou · 2022-03-21T13:44:29Z

format/Flight.proto

 * An opaque identifier that the service can use to retrieve a particular
 * portion of a stream.
+ *
+ * Tickets are meant to be single use. It is an error/undefined behavior


"undefined behavior" sounds a bit scary given the baggage this term has in C/C++. Perhaps say that the behavior can be implement-dependent and that implementations are allowed to raise an error?

lidavidm · 2022-03-21T17:51:24Z

I filed ARROW-15987 for the CI failure (this happens to HEAD as well)

jacques-n · 2022-03-21T18:01:15Z

Thanks for working through this @lidavidm!

alamb

The changes to format/Flight.proto I think are a significant clarification. Thank you

alamb · 2022-03-23T20:09:00Z

format/Flight.proto

+   * at one of the given locations, and not (necessarily) on the
+   * current service.
+   *
+   * In other words, an application can use multiple locations to


this clarification is very helpful to me

ursabot · 2022-03-24T20:12:30Z

Benchmark runs are scheduled for baseline = 012ae6e and contender = a17137f. a17137f is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Failed ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.71% ⬆️0.0%] test-mac-arm
[Failed ⬇️0.36% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

… FlightEndpoint.locations Clarify that an empty list of locations means we should fetch data from the same server, and that multiple locations mean that all locations are equivalent. See ["[Java] [Flight] Usage of Locations on FlightEndpoint"](https://lists.apache.org/thread/1668fk1myqf8168xh296qck5fn8ztcmn) on user@ for the original discussion. Closes apache#12636 from lidavidm/arrow-15921 Authored-by: David Li <[email protected]> Signed-off-by: David Li <[email protected]>

lidavidm added 3 commits March 15, 2022 11:14

ARROW-15921: [Format][FlightRPC] Clarify FlightEndpoint.locations int…

ac0648e

…erpretation

ARROW-15921: [C++][FlightRPC] Properly interpret FlightEndpoint.locat…

6ade353

…ions

ARROW-15921: [Java][FlightRPC] Properly interpret FlightEndpoint.loca…

ea7dbb8

…tions

github-actions bot added Component: FlightRPC Component: C++ Component: Java labels Mar 15, 2022

BryanCutler approved these changes Mar 15, 2022

View reviewed changes

lidavidm added 2 commits March 16, 2022 08:21

ARROW-15921: [Java][FlightRPC] Remove println statements

7d213d9

ARROW-15921: [Format][FlightRPC] Clarify Ticket usage

d6d7044

BryanCutler reviewed Mar 17, 2022

View reviewed changes

jduo reviewed Mar 17, 2022

View reviewed changes

lidavidm added 2 commits March 17, 2022 18:14

ARROW-15921: [Format][FlightRPC] Clarify endpoints/locations

8e0b2c6

ARROW-15921: [Format][FlightRPC] Clarify endpoint ordering

11bdb7b

jacques-n approved these changes Mar 18, 2022

View reviewed changes

pitrou reviewed Mar 21, 2022

View reviewed changes

ARROW-15921: [Format][FlightRPC] Clarify redemption/single-use

ca0a6ae

pitrou approved these changes Mar 21, 2022

View reviewed changes

lidavidm mentioned this pull request Mar 23, 2022

RFC: Add inlined data to flight. #12571

Closed

alamb reviewed Mar 23, 2022

View reviewed changes

lidavidm closed this in a17137f Mar 24, 2022

lidavidm deleted the arrow-15921 branch March 24, 2022 20:01

lidavidm mentioned this pull request Jul 25, 2022

ARROW-15452: [FlightRPC][Java] JDBC driver for Flight SQL #12830

Merged

ARROW-15921: [Format][FlightRPC][C++][Java] Clarify interpretation of FlightEndpoint.locations #12636

ARROW-15921: [Format][FlightRPC][C++][Java] Clarify interpretation of FlightEndpoint.locations #12636

Uh oh!

Conversation

lidavidm commented Mar 15, 2022

Uh oh!

github-actions bot commented Mar 15, 2022

Uh oh!

BryanCutler left a comment

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!

Uh oh!

Uh oh!

BryanCutler left a comment

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!

jacques-n left a comment

Choose a reason for hiding this comment

Uh oh!

lidavidm commented Mar 18, 2022

Uh oh!

pitrou left a comment

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!

lidavidm commented Mar 21, 2022

Uh oh!

jacques-n commented Mar 21, 2022

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ursabot commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

ursabot commented Mar 24, 2022 •

edited

Loading