-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-40345: [FlightRPC][C++][Java][Go] Add URI scheme to reuse connection #40084
Conversation
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?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @lidavidm -- this is a valuable feature in my mind.
My only concern is about the terminology -- Fallback
is confusing to me as it implies this scheme should only be used when other schemes fail, but I can see cases where the server intends the client to be able to fetch data from the same endpoint as a valid option (not a a fallback in an error case)
Here are some other potential names for consideration that I think may match the intent better:
arrow-flight-generator:// (matches existing terminology)
arrow-flight-default://
arrow-flight-source-location://
arrow-flight-any://
parsed by both implementations, as well as Go's ``net/url`` and | ||
Python's ``urllib.parse``.) | ||
|
||
This allows the server to return "itself" as one possible location to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend we put this context and use directly into the Flight protobuf as well so that the spec can be implemented directly from the .proto spec
Lines 436 to 452 in e83295b
/* | |
* A list of URIs where this ticket can be redeemed via DoGet(). | |
* | |
* If the list is empty, the expectation is that the ticket can only | |
* be redeemed on the current service where the ticket was | |
* generated. | |
* | |
* 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 this case, the ticket may only be redeemed | |
* at one of the given locations, and not (necessarily) on the | |
* current service. | |
* | |
* In other words, an application can use multiple locations to | |
* represent redundant and/or load balanced services. | |
*/ | |
repeated Location location = 2; |
I've already renamed it and will update the docs. Is the proposed scheme acceptable? |
I think |
1ee1d0a
to
b9b4352
Compare
b9b4352
to
a5ffc83
Compare
a5ffc83
to
2b0d38f
Compare
2b0d38f
to
52ca72a
Compare
52ca72a
to
16e24dc
Compare
|
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
16e24dc
to
a17feb1
Compare
Any final comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit ef6ea6b. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
…nnection (apache#40084) ### Rationale for this change https://docs.google.com/document/d/1g9M9FmsZhkewlT1mLibuceQO8ugI0-fqumVAXKFjVGg/edit?usp=sharing ### What changes are included in this PR? Base implementation sans integration test ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#40345 Lead-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: David Li <li.davidm96@gmail.com>
…nnection (apache#40084) ### Rationale for this change https://docs.google.com/document/d/1g9M9FmsZhkewlT1mLibuceQO8ugI0-fqumVAXKFjVGg/edit?usp=sharing ### What changes are included in this PR? Base implementation sans integration test ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#40345 Lead-authored-by: David Li <li.davidm96@gmail.com> Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: David Li <li.davidm96@gmail.com>
Rationale for this change
https://docs.google.com/document/d/1g9M9FmsZhkewlT1mLibuceQO8ugI0-fqumVAXKFjVGg/edit?usp=sharing
What changes are included in this PR?
Base implementation sans integration test
Are these changes tested?
Yes
Are there any user-facing changes?
No